[
https://issues.apache.org/jira/browse/WW-5701?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Lukasz Lenart updated WW-5701:
------------------------------
Fix Version/s: 7.4.0
> CollectionConverter silently drops a legitimate element whose text equals the
> NO_CONVERSION_POSSIBLE marker
> -----------------------------------------------------------------------------------------------------------
>
> Key: WW-5701
> URL: https://issues.apache.org/jira/browse/WW-5701
> Project: Struts 2
> Issue Type: Bug
> Reporter: Lukasz Lenart
> Assignee: Lukasz Lenart
> Priority: Major
> Fix For: 7.4.0
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> h2. Summary
> CollectionConverter decides whether an element converted successfully by
> comparing the converted value to the conversion-failure marker declared in
> TypeConverter (line 49), using equals() rather than reference identity. The
> marker's value is the ordinary text "ognl.NoConversionPossible", so a
> legitimate element whose content happens to be that text is mistaken for a
> conversion failure and silently dropped from the resulting collection.
> h2. Reproduced
> On main at 05ad78a06. A plain action property declared as a List of String,
> bound from a normal multi-valued request parameter:
> {noformat}
> vs.setValue("names", new String[]{"alpha", "ognl.NoConversionPossible",
> "omega"});
> result: [alpha, omega]
> expected: [alpha, ognl.NoConversionPossible, omega]
> {noformat}
> The middle element is discarded. No conversion error is registered, because
> no conversion actually failed - the element converted fine and was then
> thrown away by the guard. So the loss is entirely silent: the action sees a
> shorter collection with no indication anything happened.
> h2. Where
> Three sites in
> core/src/main/java/org/apache/struts2/conversion/impl/CollectionConverter.java,
> all comparing against the marker with equals():
> * line 64 - the array branch
> * line 75 - the Collection branch
> * line 83 - the single-value branch
> Note the exposure is wider than "collections explicitly declared to hold
> Strings". At lines 48-50 of the same file, when no element type can be
> determined the member type defaults to String.class, so an untyped collection
> is affected too.
> h2. Why identity is correct
> The marker field is declared as Object, not String (TypeConverter.java line
> 49), so it is not a JLS constant variable and is not inlined at compile time
> - every reader loads the one shared instance at runtime, third-party
> converters included. A parameter value built by a servlet container from
> request bytes is a distinct object even when its content is identical, so
> reference comparison distinguishes "the converter signalled failure" from
> "the user submitted this text", which is exactly the distinction being made
> here.
> OGNL itself compares this marker by identity: its own conversion loop
> compiles to a reference-comparison bytecode instruction against the same
> field, not an equals() call.
> To be precise about the limit: reference comparison protects values arriving
> from a request, which is the case that matters here. It does not protect a
> value that happens to be interned, since all identical String literals share
> one instance - application code calling the converter programmatically with
> such a literal would still lose it. Closing that as well would mean giving
> the marker an identity no user string can share, which changes a published
> constant and is a binary-compatibility question rather than a bug fix.
> h2. Proposed fix
> Replace equals() with reference comparison at all three sites.
> h2. Relationship to WW-5700
> WW-5700 fixed the mirror-image defect in XWorkMapPropertyAccessor and
> XWorkListPropertyAccessor, where the marker was stored into a typed
> collection instead of being skipped; that fix deliberately used reference
> comparison for the reason given above. CollectionConverter is the older code
> that already had a guard, but used the weaker comparison. The two were found
> together during review of WW-5700 (PR #1873) and are separate defects:
> WW-5700 stores something it should not, this one drops something it should
> keep.
> h2. Backward compatibility
> Narrow, and strictly a fix. The only behaviour that changes is that an
> element whose text is exactly "ognl.NoConversionPossible" is now kept rather
> than discarded. Nothing can reasonably depend on the current silent removal.
> h2. Status
> Fixed in PR https://github.com/apache/struts/pull/1874 - two regression
> tests, written test-first and mutation-checked. The fixture for the
> kept-element test is built at runtime rather than written as a String
> literal, because a literal would be interned to the same instance as the
> marker and the test would pass vacuously; an assertNotSame guards that. Full
> core suite green.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
