[
https://issues.apache.org/jira/browse/WW-5701?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Lukasz Lenart updated WW-5701:
------------------------------
Description:
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.
was:
h2. Summary
CollectionConverter decides whether an element converted successfully by
comparing the converted value to the marker constant
TypeConverter.NO\_CONVERSION\_POSSIBLE with 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 using equals() against the marker:
* 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 constant is declared as Object, not String (TypeConverter.java:49), so it
is not a JLS constant variable and is not inlined at compile time - every
reader loads the one shared instance. A value arriving from an HTTP request 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 an if\_acmpne against the same field.
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 identity 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.
> 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
> Time Spent: 10m
> 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)
