[
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 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.
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 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
> Priority: Major
>
> 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.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
