Skip to content

Commit

Permalink
Merge pull request #4479 from sgflt/issue-4280-improper-session-synch…
Browse files Browse the repository at this point in the history 
…ronization

 FISH-766 Improper session synchronization of session map
  • Loading branch information
@MeroRai
MeroRai authored Nov 18, 2020
2 parents 39d14da + bef9a82 commit 0892dff
Show file tree
Hide file tree
Showing 9 changed files with 190 additions and 242 deletions.
110 changes: 56 additions & 54 deletions appserver/web/web-core/src/main/java/org/apache/catalina/Manager.java
View file

Large diffs are not rendered by default.

33 changes: 15 additions & 18 deletions appserver/web/web-core/src/main/java/org/apache/catalina/authenticator/SingleSignOn.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,11 @@
*/
// Portions Copyright [2016-2019] [Payara Foundation and/or its affiliates]
package org.apache.catalina.authenticator;

import java.io.IOException;
import java.security.Principal;
import java.text.MessageFormat;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.logging.Level;

import javax.servlet.ServletException;
Expand Down Expand Up @@ -119,7 +118,7 @@ public class SingleSignOn extends ValveBase
* The cache of SingleSignOnEntry instances for authenticated Principals, keyed by the cookie value that is used to
* select them.
*/
protected Map<String, SingleSignOnEntry> cache = new HashMap<String, SingleSignOnEntry>();
protected final ConcurrentMap<String, SingleSignOnEntry> cache = new ConcurrentHashMap<>();

/**
* The lifecycle event support for this component.
Expand Down Expand Up @@ -434,9 +433,7 @@ protected void deregister(String ssoId, Session session) {

// see if we are the last session, if so blow away ssoId
if (sso.isEmpty()) {
synchronized (cache) {
cache.remove(ssoId);
}
this.cache.remove(ssoId);
}
}

Expand All @@ -449,17 +446,21 @@ protected void deregister(String ssoId, Session session) {
* @param username Username used to authenticate this user
* @param password Password used to authenticate this user
*/
protected void register(String ssoId, Principal principal, String authType, String username, char[] password, String realmName) {

protected void register(
final String ssoId,
final Principal principal,
final String authType,
final String username,
final char[] password,
final String realmName
) {
if (debug >= 1) {
String msg = MessageFormat.format(rb.getString(LogFacade.REGISTERING_SSO_INFO),
new Object[] { ssoId, principal.getName(), authType });
log(msg);
}
synchronized (cache) {
cache.put(ssoId, new SingleSignOnEntry(ssoId, 0L, principal, authType, username, realmName));
}

this.cache.put(ssoId, new SingleSignOnEntry(ssoId, 0L, principal, authType, username, realmName));
}

// ------------------------------------------------------ Protected Methods
Expand Down Expand Up @@ -501,12 +502,8 @@ protected void log(String message, Throwable t) {
*
* @param ssoId Single sign on identifier to look up
*/
protected SingleSignOnEntry lookup(String ssoId) {

synchronized (cache) {
return cache.get(ssoId);
}

protected SingleSignOnEntry lookup(final String ssoId) {
return this.cache.get(ssoId);
}

/**
Expand Down
5 changes: 2 additions & 3 deletions ...erver/web/web-core/src/main/java/org/apache/catalina/session/CookiePersistentManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,6 @@ public Session findSession(String id, HttpServletRequest request) throws IOExcep
if (cookies == null) {
return null;
}
String value = null;
for (Cookie cookie : cookies) {
if (cookieName.equals(cookie.getName())) {
return parseSession(cookie.getValue(), request.getRequestedSessionId());
Expand All @@ -112,8 +111,8 @@ public void clearSessions() {
}

@Override
public Session[] findSessions() {
return null;
public List<Session> findSessions() {
return Collections.emptyList();
}

@Override
Expand Down
16 changes: 4 additions & 12 deletions appserver/web/web-core/src/main/java/org/apache/catalina/session/ManagerBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ public abstract class ManagerBase implements Manager {
* The set of currently active Sessions for this Manager, keyed by
* session identifier.
*/
protected Map<String, Session> sessions = new ConcurrentHashMap<String, Session>();
protected final Map<String, Session> sessions = new ConcurrentHashMap<>();

// Number of sessions created by this manager
protected int sessionCounter=0;
Expand Down Expand Up @@ -896,19 +896,11 @@ public void clearSessions() {
}


/**
* Return the set of active Sessions associated with this Manager.
* If this Manager has no active Sessions, a zero-length array is returned.
*/
@Override
public Session[] findSessions() {
public List<Session> findSessions() {
// take a snapshot
Collection<Session> sessionsValues = sessions.values();
List<Session> list = new ArrayList<Session>(sessionsValues.size());
for (Session session : sessionsValues) {
list.add(session);
}
return list.toArray(new Session[list.size()]);
Collection<Session> sessionsValues = this.sessions.values();
return new ArrayList<>(sessionsValues);
}


Expand Down
81 changes: 30 additions & 51 deletions appserver/web/web-core/src/main/java/org/apache/catalina/session/PersistentManagerBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -594,21 +594,14 @@ public void clearStore() {
* Hercules: modified method
*/
protected void processExpires() {

if (!started)
return;

Session sessions[] = findSessions();

for (Session session1 : sessions) {
final List<Session> sessions = findSessions();
for (final Session session1 : sessions) {
StandardSession session = (StandardSession) session1;
/* START CR 6363689
if (!session.isValid()) {
*/
// START CR 6363689
if(!session.getIsValid() || session.hasExpired()) {
// END CR 6363689
if(session.lockBackground()) {
if(session.lockBackground()) {
try {
session.expire();
} finally {
Expand Down Expand Up @@ -951,20 +944,21 @@ public void unload() {
if (store == null)
return;

Session sessions[] = findSessions();
int n = sessions.length;
if (n == 0)
final List<Session> sessions = findSessions();
if (sessions.isEmpty())
return;

if (log.isLoggable(Level.FINE)) {
log.log(Level.FINE, LogFacade.SAVING_PERSISTED_SESSION, String.valueOf(n));
log.log(Level.FINE, LogFacade.SAVING_PERSISTED_SESSION, sessions.size());
}
for (int i = 0; i < n; i++)

for (final Session session : sessions) {
try {
swapOut(sessions[i]);
swapOut(session);
} catch (IOException e) {
// This is logged in writeSession()
}
}

}

Expand Down Expand Up @@ -1261,11 +1255,12 @@ public void stop() throws LifecycleException {
unload();
} else {
// Expire all active sessions
Session sessions[] = findSessions();
for (Session session1 : sessions) {
StandardSession session = (StandardSession) session1;
if (!session.isValid())
final List<Session> sessions = findSessions();
for (final Session session1 : sessions) {
final StandardSession session = (StandardSession) session1;
if (!session.isValid()) {
continue;
}
session.expire();
}
}
Expand Down Expand Up @@ -1320,15 +1315,15 @@ protected void processMaxIdleSwaps() {
if (!isStarted() || maxIdleSwap < 0)
return;

Session sessions[] = findSessions();
long timeNow = System.currentTimeMillis();
final List<Session> sessions = findSessions();
final long timeNow = System.currentTimeMillis();

// Swap out all sessions idle longer than maxIdleSwap
// FIXME: What's preventing us from mangling a session during
// a request?
if (maxIdleSwap >= 0) {
for (Session session1 : sessions) {
StandardSession session = (StandardSession) session1;
for (final Session session1 : sessions) {
final StandardSession session = (StandardSession) session1;
if (!session.isValid())
continue;
int timeIdle = // Truncate, do not round up
Expand All @@ -1354,40 +1349,35 @@ protected void processMaxIdleSwaps() {
* Hercules: modified method
*/
protected void processMaxActiveSwaps() {

if (!isStarted() || getMaxActiveSessions() < 0)
return;

Session sessions[] = findSessions();
final List<Session> sessions = findSessions();

// FIXME: Smarter algorithm (LRU)
if (getMaxActiveSessions() >= sessions.length)
if (sessions.size() <= getMaxActiveSessions())
return;

if(log.isLoggable(Level.FINE)) {
log.log(Level.FINE, LogFacade.TOO_MANY_ACTIVE_SESSION, sessions.length);
log.log(Level.FINE, LogFacade.TOO_MANY_ACTIVE_SESSION, sessions.size());
}
int toswap = sessions.length - getMaxActiveSessions();
int toswap = sessions.size() - getMaxActiveSessions();
long timeNow = System.currentTimeMillis();

for (int i = 0; i < sessions.length && toswap > 0; i++) {
for (int i = 0; i < sessions.size() && toswap > 0; i++) {
int timeIdle = // Truncate, do not round up
(int) ((timeNow - sessions[i].getLastAccessedTime()) / 1000L);
(int) ((timeNow - sessions.get(i).getLastAccessedTime()) / 1000L);
if (timeIdle > minIdleSwap) {
StandardSession session = (StandardSession) sessions[i];
StandardSession session = (StandardSession) sessions.get(i);
//skip the session if it cannot be locked
if(session.lockBackground()) {
if(log.isLoggable(Level.FINE)) {
log.log(Level.FINE, LogFacade.SWAP_OUT_SESSION, new Object[] {session.getIdInternal(), timeIdle});
}
try {
swapOut(session);
} catch (java.util.ConcurrentModificationException e1) {
// This is logged in writeSession()
} catch (IOException e) {
// This is logged in writeSession()
} catch (Exception e) {
// This is logged in writeSession()
// This is logged in writeSession()
} finally {
session.unlockBackground();
}
Expand All @@ -1408,8 +1398,8 @@ protected void processMaxIdleBackups() {
if (!isStarted() || maxIdleBackup < 0)
return;

Session sessions[] = findSessions();
long timeNow = System.currentTimeMillis();
final List<Session> sessions = findSessions();
final long timeNow = System.currentTimeMillis();

// Back up all sessions idle longer than maxIdleBackup
if (maxIdleBackup >= 0) {
Expand All @@ -1427,12 +1417,8 @@ protected void processMaxIdleBackups() {
}
try {
writeSession(session);
} catch (java.util.ConcurrentModificationException e1) {
// This is logged in writeSession()
} catch (IOException e) {
// This is logged in writeSession()
} catch (Exception e) {
// This is logged in writeSession()
// This is logged in writeSession()
} finally {
session.unlockBackground();
}
Expand All @@ -1442,11 +1428,4 @@ protected void processMaxIdleBackups() {
}

}

public String getMonitorAttributeValues() {
//FIXME if desired for monitoring 'file'
return "";
}


}
57 changes: 26 additions & 31 deletions appserver/web/web-core/src/main/java/org/apache/catalina/session/StandardManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -669,24 +669,23 @@ public void writeSessions(OutputStream os, boolean doExpire)
}

// Write the number of active sessions, followed by the details
StandardSession[] currentStandardSessions = null;
final StandardSession[] currentStandardSessions;
synchronized (sessions) {
if (log.isLoggable(Level.FINE))
log.log(Level.FINE, "Unloading {0} sessions", sessions.size());
try {
// START SJSAS 6375689
for (Session actSession : findSessions()) {
for (final Session actSession : findSessions()) {
StandardSession session = (StandardSession) actSession;
session.passivate();
}
// END SJSAS 6375689
Session[] currentSessions = findSessions();
int size = currentSessions.length;
final List<Session> currentSessions = findSessions();
int size = currentSessions.size();
currentStandardSessions = new StandardSession[size];
oos.writeObject(size);
for (int i = 0; i < size; i++) {
StandardSession session =
(StandardSession) currentSessions[i];
final StandardSession session = (StandardSession) currentSessions.get(i);
currentStandardSessions[i] = session;
/* SJSAS 6375689
session.passivate();
Expand Down Expand Up @@ -882,21 +881,19 @@ public void stop(boolean isShutdown) throws LifecycleException {
}

// Expire all active sessions and notify their listeners
Session sessions[] = findSessions();
if (sessions != null) {
for (Session session : sessions) {
if (!session.isValid()) {
continue;
}
try {
session.expire();
} catch (Throwable t) {
// Ignore
} finally {
// Measure against memory leaking if references to the session
// object are kept in a shared field somewhere
session.recycle();
}
final List<Session> sessions = findSessions();
for (final Session session : sessions) {
if (!session.isValid()) {
continue;
}
try {
session.expire();
} catch (Throwable t) {
// Ignore
} finally {
// Measure against memory leaking if references to the session
// object are kept in a shared field somewhere
session.recycle();
}
}

Expand Down Expand Up @@ -985,16 +982,14 @@ public void processExpires() {

long timeNow = System.currentTimeMillis();

Session[] sessions = findSessions();
if (sessions != null) {
for (Session session : sessions) {
StandardSession sess = (StandardSession) session;
if (sess.lockBackground()) {
try {
sess.isValid();
} finally {
sess.unlockBackground();
}
final List<Session> sessions = findSessions();
for (final Session session : sessions) {
final StandardSession sess = (StandardSession) session;
if (sess.lockBackground()) {
try {
sess.isValid();
} finally {
sess.unlockBackground();
}
}
}
Expand Down
Loading

0 comments on commit 0892dff

Please sign in to comment.