Re: [PR] [#2713] [3.0] bugfix: using atomics for session updates [shiro]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #2712:
URL: https://github.com/apache/shiro/pull/2712#discussion_r3293827691


##########
core/src/main/java/org/apache/shiro/session/mgt/SimpleSession.java:
##########
@@ -194,17 +198,15 @@ public Map<Object, Object> getAttributes() {
     }
 
     public void setAttributes(Map<Object, Object> attributes) {

Review Comment:
   `setAttributes` now unconditionally copies into a `ConcurrentHashMap`. This will throw a NullPointerException if callers pass `null` (previously allowed to clear attributes) and will also fail if the supplied map contains null keys/values (ConcurrentHashMap forbids them). Consider preserving the prior contract by allowing `null` to clear the field and either validating keys/values with a clear exception or documenting/enforcing non-null keys explicitly.
   



##########
core/src/main/java/org/apache/shiro/session/mgt/SimpleSession.java:
##########
@@ -301,10 +303,8 @@ public void validate() throws InvalidSessionException {
     }
 
     private Map<Object, Object> getAttributesLazy() {
-        Map<Object, Object> attributes = getAttributes();
         if (attributes == null) {
-            attributes = new HashMap<Object, Object>();
-            setAttributes(attributes);
+            attributes = new ConcurrentHashMap<>();
         }
         return attributes;

Review Comment:
   `getAttributesLazy` is not thread-safe: if multiple threads call it when `attributes` is null, one thread can create and populate a map that is then overwritten by another thread's new `ConcurrentHashMap`, losing updates. If this class is expected to be safely used concurrently (as implied by the atomic changes), consider making initialization atomic (e.g., synchronize/double-check, use an `AtomicReference`, or eagerly initialize `attributes`).



##########
core/src/main/java/org/apache/shiro/session/mgt/SimpleSession.java:
##########
@@ -491,16 +491,16 @@ private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundE
             this.startTimestamp = (Date) in.readObject();
         }
         if (isFieldPresent(bitMask, STOP_TIMESTAMP_BIT_MASK)) {
-            this.stopTimestamp = (Date) in.readObject();
+            this.stopTimestamp = new AtomicReference<>((Date) in.readObject());
         }
         if (isFieldPresent(bitMask, LAST_ACCESS_TIME_BIT_MASK)) {
-            this.lastAccessTime = (Date) in.readObject();
+            this.lastAccessTime = new AtomicReference<>((Date) in.readObject());

Review Comment:
   `readObject` only initializes the new Atomic* fields when the corresponding bit is present. When a bit is not present (e.g., STOP_TIMESTAMP/EXPIRED when they are at default values), these transient fields remain null after deserialization because constructors/field initializers are not run, leading to NullPointerExceptions in methods like `getStopTimestamp()`, `isExpired()`, `stop()`, `writeObject()`, and `getAlteredFieldsBitMask()`. Initialize `stopTimestamp`, `lastAccessTime`, `timeout`, and `expired` to safe defaults at the start of `readObject` (and then overwrite when bits are present).



##########
core/src/main/java/org/apache/shiro/session/mgt/SimpleSession.java:
##########
@@ -491,16 +491,16 @@ private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundE
             this.startTimestamp = (Date) in.readObject();
         }
         if (isFieldPresent(bitMask, STOP_TIMESTAMP_BIT_MASK)) {
-            this.stopTimestamp = (Date) in.readObject();
+            this.stopTimestamp = new AtomicReference<>((Date) in.readObject());

Review Comment:
   There are serialization tests for `SimpleSession`, but they don’t currently cover cases where optional fields are *not* present in the bitmask (e.g., default `expired=false` and `stopTimestamp=null`). Adding a test that serializes/deserializes a default session and then calls `isExpired()`, `stop()`, and `getStopTimestamp()` would catch null-initialization regressions introduced by the new Atomic* fields.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org