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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
