Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FISH-766 Improper session synchronization of session map #4479

Merged
merged 8 commits into from
Nov 18, 2020
Merged

FISH-766 Improper session synchronization of session map #4479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5d6e7cd
#4280 Fixed synchronization issue
sgflt Feb 9, 2020
3215ce9
#4281 Added final modifier to field
sgflt Feb 9, 2020
6c64922
Merge remote-tracking branch 'upstream/master' into issue-4280-improp…
sgflt Jul 19, 2020
b298f29
#4280 Replaced HashMap by ConcurrentHashMap in SingleSignOn
sgflt Jul 19, 2020
a00f866
#4280 clean up code in GlassFishSingleSignOn
sgflt Jul 19, 2020
1754479
#4280 Fixed unwanted double expiration
sgflt Jul 19, 2020
dc43453
#4280 Deleted unused variable
sgflt Jul 19, 2020
bef9a82
#4280 Fixed contract violation of findSessions
sgflt Jul 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 56 additions & 54 deletions appserver/web/web-core/src/main/java/org/apache/catalina/Manager.java
View file
Open in desktop

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
Open in desktop
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
Open in desktop
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;
Copy link
Contributor Author

@sgflt sgflt Jul 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

broken contract fixed

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
Open in desktop
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
Open in desktop
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
Open in desktop
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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nullcheck for broken contract removed

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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nullcheck for broken contract removed

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