Copilot commented on code in PR #2712:
URL: https://github.com/apache/shiro/pull/2712#discussion_r3293868992
##########
core/src/main/java/org/apache/shiro/session/mgt/SimpleSession.java:
##########
@@ -451,21 +458,31 @@ private void writeObject(ObjectOutputStream out) throws
IOException {
if (startTimestamp != null) {
out.writeObject(startTimestamp);
}
+
+ var stopTimestamp = getStopTimestamp();
if (stopTimestamp != null) {
out.writeObject(stopTimestamp);
}
+
+ var lastAccessTime = getLastAccessTime();
if (lastAccessTime != null) {
out.writeObject(lastAccessTime);
}
+
+ var timeout = getTimeout();
if (timeout != 0L) {
out.writeLong(timeout);
}
+
+ var expired = isExpired();
if (expired) {
out.writeBoolean(expired);
}
Review Comment:
In `writeObject`, the decision to write
`stopTimestamp`/`lastAccessTime`/`timeout`/`expired` is based on freshly read
values, but the deserializer relies on the earlier `alteredFieldsBitMask`. If
any of these values changes between the bitmask calculation and these writes
(now more plausible due to atomic updates), the stream can become inconsistent
with the bitmask and deserialization can fail. To avoid this, snapshot all
serialized fields into locals first, compute the bitmask from that snapshot,
and write strictly from the snapshot.
##########
core/src/main/java/org/apache/shiro/session/mgt/SimpleSession.java:
##########
@@ -91,18 +94,19 @@ public class SimpleSession implements ValidatingSession,
Serializable {
// ==============================================================
private transient Serializable id;
private transient Date startTimestamp;
- private transient Date stopTimestamp;
- private transient Date lastAccessTime;
- private transient long timeout;
- private transient boolean expired;
+ private transient AtomicReference<Date> stopTimestamp;
+ private transient AtomicReference<Date> lastAccessTime;
+ private transient AtomicLong timeout;
+ private transient AtomicBoolean expired = new AtomicBoolean();
private transient String host;
- private transient Map<Object, Object> attributes;
+ private transient volatile Map<Object, Object> attributes;
public SimpleSession() {
//TODO - remove concrete reference to DefaultSessionManager
- this.timeout = DefaultSessionManager.DEFAULT_GLOBAL_SESSION_TIMEOUT;
+ this.timeout = new
AtomicLong(DefaultSessionManager.DEFAULT_GLOBAL_SESSION_TIMEOUT);
this.startTimestamp = new Date();
- this.lastAccessTime = this.startTimestamp;
+ this.stopTimestamp = new AtomicReference<>();
+ this.lastAccessTime = new AtomicReference<>(this.startTimestamp);
}
Review Comment:
This change is addressing a concurrency/visibility issue (#2713), but there
is no regression test exercising concurrent `touch()`/`validate()` (or similar
session updates) to demonstrate the fix and prevent reintroduction. Consider
adding a multi-threaded test (e.g., repeatedly touching while validating over
many iterations) that would fail under the pre-fix implementation on
weak-memory architectures and pass with the atomic/volatile updates.
##########
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) {
- this.attributes = attributes;
+ this.attributes = attributes == null ? null : new
ConcurrentHashMap<>(attributes);
}
Review Comment:
`setAttributes` now always copies into a new `ConcurrentHashMap`, which
changes the behavior of this public setter (callers no longer retain their
provided `Map` instance/type) and can be an unnecessary full copy for large
maps. Consider preserving the input when it is already a
`ConcurrentHashMap`/`ConcurrentMap` (or at least documenting this behavior
change), and ensure deserialization also normalizes `attributes` the same way
so the field is consistently thread-safe after `readObject`.
--
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]
