jackson-core icon indicating copy to clipboard operation
jackson-core copied to clipboard

Fixes Implicit narrowing conversion in compound assignment FloatToDecimal

Open odaysec opened this issue 6 months ago • 3 comments

https://github.com/FasterXML/jackson-core/blob/6affde88dd3820b8c2eeb8700ae24b75651c8fcd/src/main/java/com/fasterxml/jackson/core/io/schubfach/FloatToDecimal.java#L408-L408 https://github.com/FasterXML/jackson-core/blob/6affde88dd3820b8c2eeb8700ae24b75651c8fcd/src/main/java/com/fasterxml/jackson/core/io/schubfach/FloatToDecimal.java#L415-L415 https://github.com/FasterXML/jackson-core/blob/6affde88dd3820b8c2eeb8700ae24b75651c8fcd/src/main/java/com/fasterxml/jackson/core/io/schubfach/FloatToDecimal.java#L426-L426 https://github.com/FasterXML/jackson-core/blob/6affde88dd3820b8c2eeb8700ae24b75651c8fcd/src/main/java/com/fasterxml/jackson/core/io/schubfach/FloatToDecimal.java#L440-L440

Fix the issue need to ensure that the type of the left-hand side (f) is at least as wide as the type of the right-hand side (pow10(H - len)). Since pow10 likely returns a long, the simplest and safest fix is to change the type of f from int to long. This avoids the implicit narrowing conversion and ensures that the computation can handle larger values without overflow.

Steps to implement the fix:

  1. Change the type of f from int to long in the method toChars.
  2. Update any other code within the method that interacts with f to ensure compatibility with the new type.
  3. Verify that the change does not introduce any unintended side effects or performance issues.

Compound assignment statements of the form x += y or x *= y perform an implicit narrowing conversion if the type of x is narrower than the type of y. x += y is equivalent to x = (T)(x + y), where T is the type of x. This can result in information loss and numeric errors such as overflows.

References

Compound Assignment Operators, Narrowing Primitive Conversion NUM00-J. Detect or prevent integer overflow

odaysec avatar Jun 10 '25 20:06 odaysec

@odaysec can you provide a test case that demos that this is a fix? A test case that fails with the existing code and that is fixed by your change.

pjfanning avatar Jun 10 '25 20:06 pjfanning

Yeah we definitely need something to show a problem to solve, not a speculative fix without one.

This has bit of a smell of tool/scanner reported concern.

Also worth noting that code for "schubfach" is an outside contribution so if there's an issue, should probably be fixed there (but I don't remember specifically where it's from, @pjfanning may know).

cowtowncoder avatar Jun 11 '25 00:06 cowtowncoder

Yeah we definitely need something to show a problem to solve, not a speculative fix without one.

This has bit of a smell of tool/scanner reported concern.

Also worth noting that code for "schubfach" is an outside contribution so if there's an issue, should probably be fixed there (but I don't remember specifically where it's from, @pjfanning may know).

Comes from here: https://github.com/c4f7fcce9cb06515/Schubfach

pjfanning avatar Jun 11 '25 00:06 pjfanning

No reproduction of an actual issue to fix, closing. May be re-opened/re-filed with some proof.

cowtowncoder avatar Aug 13 '25 17:08 cowtowncoder