This is an automated email from the ASF dual-hosted git repository.
markt-asf pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new de25bedd07 Follow-up to "Validate the principal object graph before
persisting..."
de25bedd07 is described below
commit de25bedd07f259642501bdaeb998ba0bb635dbd0
Author: Mark Thomas <[email protected]>
AuthorDate: Fri Oct 16 11:35:24 2026 +0100
Follow-up to "Validate the principal object graph before persisting..."
See the diff for the full explanation. The short version is that this
was an expensive check paid by everyone for a rare bug that should
affect very few - hence the refactoring.
---
.../apache/catalina/ha/session/DeltaSession.java | 21 ++++++++-
.../catalina/ha/session/LocalStrings.properties | 2 +
.../apache/catalina/session/StandardSession.java | 52 ++++++++++++++--------
3 files changed, 55 insertions(+), 20 deletions(-)
diff --git a/java/org/apache/catalina/ha/session/DeltaSession.java
b/java/org/apache/catalina/ha/session/DeltaSession.java
index 6e7dc8e1e2..ab109bfbbd 100644
--- a/java/org/apache/catalina/ha/session/DeltaSession.java
+++ b/java/org/apache/catalina/ha/session/DeltaSession.java
@@ -874,7 +874,19 @@ public class DeltaSession extends StandardSession
implements Externalizable, Clu
boolean hasPrincipal = stream.readBoolean();
principal = null;
if (hasPrincipal) {
- principal = (Principal) stream.readObject();
+ try {
+ principal = (Principal) stream.readObject();
+ } catch (WriteAbortedException wae) {
+ if (!(wae.getCause() instanceof NotSerializableException)) {
+ throw wae;
+ }
+ String msg =
sm.getString("standardSession.principalNotDeserializable", id);
+ if (manager.getContext().getLogger().isDebugEnabled()) {
+ manager.getContext().getLogger().debug(msg, wae);
+ } else {
+ manager.getContext().getLogger().warn(msg);
+ }
+ }
}
id = (String) stream.readObject();
@@ -975,7 +987,12 @@ public class DeltaSession extends StandardSession
implements Externalizable, Clu
stream.writeObject(Long.valueOf(version));
stream.writeBoolean(getPrincipal() instanceof Serializable);
if (getPrincipal() instanceof Serializable) {
- stream.writeObject(getPrincipal());
+ // See StandardSession's notes on serializing Principal
+ try {
+ stream.writeObject(getPrincipal());
+ } catch (NotSerializableException e) {
+
manager.getContext().getLogger().warn(sm.getString("standardSession.principalNotSerializable",
id), e);
+ }
}
stream.writeObject(id);
diff --git a/java/org/apache/catalina/ha/session/LocalStrings.properties
b/java/org/apache/catalina/ha/session/LocalStrings.properties
index 0fbc72cf4a..cbce4fb798 100644
--- a/java/org/apache/catalina/ha/session/LocalStrings.properties
+++ b/java/org/apache/catalina/ha/session/LocalStrings.properties
@@ -89,5 +89,7 @@ jvmRoute.valve.started=JvmRouteBinderValve started
jvmRoute.valve.stopped=JvmRouteBinderValve stopped
standardSession.notSerializable=Cannot serialize session attribute [{0}] for
session [{1}]
+standardSession.principalNotDeserializable=Cannot deserialize Principal object
for session [{0}]
+standardSession.principalNotSerializable=Cannot serialize Principal object for
session [{0}]
standardSession.removeAttribute.ise=removeAttribute: Session already
invalidated
standardSession.setAttribute.namenull=setAttribute: name parameter cannot be
null
diff --git a/java/org/apache/catalina/session/StandardSession.java
b/java/org/apache/catalina/session/StandardSession.java
index 2fd448c824..1f3e629639 100644
--- a/java/org/apache/catalina/session/StandardSession.java
+++ b/java/org/apache/catalina/session/StandardSession.java
@@ -17,7 +17,6 @@
package org.apache.catalina.session;
import java.beans.PropertyChangeSupport;
-import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.NotSerializableException;
import java.io.ObjectInputStream;
@@ -1182,6 +1181,16 @@ public class StandardSession implements HttpSession,
Session, Serializable {
setAuthType((String) nextObject);
try {
setPrincipal((Principal) stream.readObject());
+ } catch (WriteAbortedException wae) {
+ if (!(wae.getCause() instanceof NotSerializableException)) {
+ throw wae;
+ }
+ String msg =
sm.getString("standardSession.principalNotDeserializable", id);
+ if (manager.getContext().getLogger().isDebugEnabled()) {
+ manager.getContext().getLogger().debug(msg, wae);
+ } else {
+ manager.getContext().getLogger().warn(msg);
+ }
} catch (ClassNotFoundException | ObjectStreamException e) {
String msg =
sm.getString("standardSession.principalNotDeserializable", id);
if (manager.getContext().getLogger().isDebugEnabled()) {
@@ -1301,24 +1310,31 @@ public class StandardSession implements HttpSession,
Session, Serializable {
// Write authentication information (may be null values)
stream.writeObject(sessionAuthType);
- if (sessionPrincipal != null) {
- /*
- * The instanceof check above only covers the principal itself.
- * Verify that the complete object graph is serializable before
- * writing to the session stream, since a NotSerializableException
- * thrown for a nested element mid-write would leave the session
- * record corrupted and unreadable. Discard the principal in the
- * same way as above if the graph is not serializable.
- */
- try (ByteArrayOutputStream baos = new ByteArrayOutputStream();
- ObjectOutputStream principalStream = new
ObjectOutputStream(baos)) {
- principalStream.writeObject(sessionPrincipal);
- } catch (NotSerializableException e) {
- sessionPrincipal = null;
-
manager.getContext().getLogger().warn(sm.getString("standardSession.principalNotSerializable",
id), e);
- }
+ /*
+ * The instanceof check above only covers the principal itself, not
the full object graph so serialization
+ * could still fail. If serialization fails it will trigger a
WriteAbortedException on read that also needs to
+ * be handled.
+ *
+ * There is a broader issue here. The serialization failure is
recoverable assuming that default object
+ * serialization is being used. If custom object serialization is used
and it fails then it is likely that the
+ * recovery will fail also. This situation can be detected but only by
attempting a separate serialization in
+ * a temporary byte stream which is relatively expensive since the
temporary stream is then thrown away.
+ *
+ * However, it is considered unlikely that serialization (standard or
custom) will fail and would be a bug in
+ * the Principal if it did since the Javadoc for Serializable states
that "It is an error to declare a class
+ * Serializable if this is not the case."
+ *
+ * Therefore, the "instanceof Serializable" test with handling of the
exceptions in the unlikely event that
+ * serialization does fail is considered sufficient.
+ *
+ * Note: session attributes use the the recovery mechanism and have
the same risk with respect to custom
+ * serialization.
+ */
+ try {
+ stream.writeObject(sessionPrincipal);
+ } catch (NotSerializableException e) {
+
manager.getContext().getLogger().warn(sm.getString("standardSession.principalNotSerializable",
id), e);
}
- stream.writeObject(sessionPrincipal);
stream.writeObject(expectedSessionId);
stream.writeObject(savedRequest);
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
