[
https://issues.apache.org/jira/browse/WW-5826?focusedWorklogId=1051712&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1051712
]
ASF GitHub Bot logged work on WW-5826:
--------------------------------------
Author: ASF GitHub Bot
Created on: 01/Nov/26 08:10
Start Date: 01/Nov/26 08:10
Worklog Time Spent: 10m
Work Description: lukaszlenart commented on PR #2082:
URL: https://github.com/apache/struts/pull/2082#issuecomment-6555367576
Thanks, the analysis holds: `Dispatcher` wraps the session in a new
`SessionMap` per request (`Dispatcher.java:902`), so `synchronized (o)` never
excludes two concurrent requests and the static map is mutated without a shared
monitor. The one-line fix is correct and does not interfere with the
`o.wait()`/`o.notify()` logic, since the map's own lock is only held inside
`get`/`put`/`remove`.
Worth saying what the practical effect is, because the PR describes only the
size counter: every entry left behind after `unlock()` pins a `SessionMap` (and
through it the request and session) plus the whole `ActionInvocation`. So this
is a slow leak under concurrent load for applications that use `scope`, not
just a cosmetic counter. That is the reason to merge it.
Two requests before merging:
1. **The test is too heavy for what it guards.** 8 threads × 40,000
iterations on every core build, for a one-line change, and it never fails once
the fix is in. Please cut the iterations to something small (a few hundred per
thread is enough to keep it as a regression smoke test) and drop the
reflection: the test is in the same package, so a package-private `static int
lockCount()` next to `clearLocks()` avoids reaching into a private field.
2. **The affected-versions note in WW-5826 is inaccurate.** The 6.12.0 tag
has the same pattern (`locks` is a plain `IdentityHashMap`, `lock()`/`unlock()`
synchronize on `o`, only `clearLocks()` synchronizes on `locks`), so 6.x is not
different from 7.x here. Please correct the ticket; which versions get the fix
is decided on the ticket.
Scope note: please keep the PR to this. Changing what the interceptor locks
on would be a behaviour change and is out of scope here.
Issue Time Tracking
-------------------
Worklog Id: (was: 1051712)
Time Spent: 20m (was: 10m)
> Make ScopeInterceptor lock table thread-safe
> --------------------------------------------
>
> Key: WW-5826
> URL: https://issues.apache.org/jira/browse/WW-5826
> Project: Struts 2
> Issue Type: Improvement
> Components: Core
> Affects Versions: 7.0.0, 7.3.0, 7.4.0
> Reporter: Gouri Sankar A
> Priority: Minor
> Fix For: 7.5.0
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> ScopeInterceptor keeps a static IdentityHashMap named locks. lock() and
> unlock() modify it inside synchronized (o), where o is the session map.
> Dispatcher creates a new SessionMap per request, so concurrent requests do
> not share that monitor and the map is modified without mutual exclusion.
> Under concurrent use the map's size counter was observed to be corrupted
> (positive and negative values). This is hardening; no security impact was
> demonstrated. The interceptor is opt-in: it is declared as "scope" in
> struts-default.xml but is not part of any bundled interceptor stack, so an
> application has to reference it explicitly.
> Affected versions: the same code (a static IdentityHashMap locks guarded by
> synchronized (o)) is present in the 7.0.0, 7.3.0 and 7.4.0 tags, by source
> inspection. The race itself was only reproduced on main. The 6.12.0 tag
> differs (lock() synchronizes on locks), so 6.x is not claimed.
> Fix: wrap the map with Collections.synchronizedMap, consistent with the
> synchronized (locks) in clearLocks().
> Tests: ScopeInterceptorTest#testConcurrentLockAndUnlockLeavesNoEntries. It
> failed in every run without the fix (25 runs) and passed 20/20 with it. It is
> statistical: it can pass without the fix on a fast machine.
> Pull request: https://github.com/apache/struts/pull/2082
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
