Fixes Implicit narrowing conversion in compound assignment FloatToDecimal

[odaysec]

jackson-core/src/main/java/com/fasterxml/jackson/core/io/schubfach/FloatToDecimal.java

Line 408 in 6affde8

private int toChars(int f, int e) {

jackson-core/src/main/java/com/fasterxml/jackson/core/io/schubfach/FloatToDecimal.java

Line 415 in 6affde8

int len = flog10pow2(Integer.SIZE - numberOfLeadingZeros(f));

jackson-core/src/main/java/com/fasterxml/jackson/core/io/schubfach/FloatToDecimal.java

Line 426 in 6affde8

f *= pow10(H - len);

jackson-core/src/main/java/com/fasterxml/jackson/core/io/schubfach/FloatToDecimal.java

Line 440 in 6affde8

int l = f - 100_000_000 * h;

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

[pjfanning]

@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]

I can't find any calls to this method where the input f is long - they are all int. I think there will be a performance penalty here as there will need to be an implicit cast from int to long on all calls to this method after this change.

[odaysec]

Hi @pjfanning

Thank you for the respond.

The reason I changed the parameter type from int to long was to address a potential integer overflow issue when handling large float values, particularly during conversion to a decimal representation. However, I understand the concern regarding the potential performance impact due to implicit casting from int to long on all existing calls to this method. If all current usages only involve int, I’m open to alternative approaches such as:

• Introducing an overloaded method toChars(long f, int e) without removing the existing int version, to preserve performance for current usage patterns.
• Re-evaluating whether long is truly necessary, or if overflow can be handled with validation before the call.

I’m happy to adjust the fix based on what would be best for both performance and maintainability. Please let me know your preferred direction.

[pjfanning]

I guess the start is to provide a broken test case. I'd prefer not to deal with theory and more interested in actual test cases.

[pjfanning]

the code change doesn't even compile

Error:  COMPILATION ERROR : 
Error:  /home/runner/work/jackson-core/jackson-core/src/main/java/com/fasterxml/jackson/core/io/schubfach/FloatToDecimal.java:[415,63] incompatible types: possible lossy conversion from long to int

[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).

[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).

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