Skip to content

Commit 354f486

Browse files
jandro996jandro996
authored
Fix IllegalFormatConversionException StringModuleImpl#onStringFormat (#9907)
* Fix * New approach * remove tests * sef review * sef review * review
1 parent 9dde14c commit 354f486

File tree

7 files changed

+355
-5
lines changed

7 files changed

+355
-5
lines changed

‎dd-java-agent/agent-iast/src/main/java/com/datadog/iast/propagation/StringModuleImpl.java‎

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -528,13 +528,13 @@ public void onStringFormat(
528528
final int parsedIndex = Integer.parseInt(index.substring(0, index.length() - 1)) - 1;
529529
// remove the index before the formatting without increment the current state
530530
parameter = parameters[parsedIndex];
531-
formattedValue = String.format(locale, placeholder.replace(index, ""), parameter);
531+
formattedValue = formatValue(locale, placeholder.replace(index, ""), parameter);
532532
} else {
533533
if (!checkParameterBounds(format, parameters, paramIndex)) {
534534
return; // return without tainting the string in case of error
535535
}
536536
parameter = parameters[paramIndex++];
537-
formattedValue = String.format(locale, placeholder, parameter);
537+
formattedValue = formatValue(locale, placeholder, parameter);
538538
}
539539
taintedObject = to.get(parameter);
540540
}
@@ -571,6 +571,30 @@ private static boolean checkParameterBounds(
571571
return false;
572572
}
573573

574+
/**
575+
* Formats a value using String.format with telemtry report on IllegalFormatException.
576+
*
577+
* @param locale the locale to use for formatting
578+
* @param placeholder the format placeholder (e.g., "%f", "%d")
579+
* @param parameter the parameter to format
580+
* @return the formatted value
581+
*/
582+
private static String formatValue(
583+
@Nullable final Locale locale, final String placeholder, final Object parameter) {
584+
try {
585+
return String.format(locale, placeholder, parameter);
586+
} catch (final java.util.IllegalFormatException e) {
587+
// Send telemetry to improve future bug fixes
588+
LOG.debug(
589+
SEND_TELEMETRY,
590+
"Format conversion failed for placeholder {} with parameter type {}: {}",
591+
placeholder,
592+
parameter == null ? "null" : parameter.getClass().getName(),
593+
e.getMessage());
594+
throw e;
595+
}
596+
}
597+
574598
@Override
575599
public void onStringFormat(
576600
@Nonnull final Iterable<String> literals,

‎dd-java-agent/instrumentation/scala/src/main/java/datadog/trace/instrumentation/scala/StringOpsCallSite.java‎

Lines changed: 46 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import datadog.trace.api.iast.propagation.StringModule;
88
import javax.annotation.Nonnull;
99
import scala.collection.immutable.StringOps;
10+
import scala.math.ScalaNumber;
1011

1112
@Propagation
1213
@CallSite(
@@ -28,7 +29,10 @@ public static String afterInterpolation(
2829
if (module != null) {
2930
try {
3031
final Object[] args = ScalaJavaConverters.toArray(params);
31-
module.onStringFormat(stringOps.toString(), args, result);
32+
// Unwrap Scala ScalaNumber types to their underlying Java representation
33+
// This replicates Scala's unwrapArg() behavior before calling String.format
34+
final Object[] unwrappedArgs = unwrapScalaNumbers(args);
35+
module.onStringFormat(stringOps.toString(), unwrappedArgs, result);
3236
} catch (final Throwable e) {
3337
module.onUnexpectedException("afterInterpolation threw", e);
3438
}
@@ -47,11 +51,51 @@ public static String afterInterpolation(
4751
if (module != null) {
4852
try {
4953
final Object[] args = ScalaJavaConverters.toArray(params);
50-
module.onStringFormat(pattern, args, result);
54+
// Unwrap Scala ScalaNumber types to their underlying Java representation
55+
// This replicates Scala's unwrapArg() behavior before calling String.format
56+
final Object[] unwrappedArgs = unwrapScalaNumbers(args);
57+
module.onStringFormat(pattern, unwrappedArgs, result);
5158
} catch (final Throwable e) {
5259
module.onUnexpectedException("afterInterpolation threw", e);
5360
}
5461
}
5562
return result;
5663
}
64+
65+
/**
66+
* Unwraps all Scala ScalaNumber types in an array to their underlying Java representations.
67+
*
68+
* <p>This method replicates Scala's unwrapArg behavior from StringLike.scala:
69+
*
70+
* <pre>
71+
* private def unwrapArg(arg: Any): AnyRef = arg match {
72+
* case x: ScalaNumber => x.underlying
73+
* case x => x.asInstanceOf[AnyRef]
74+
* }
75+
* </pre>
76+
*
77+
* <p>The conversion is:
78+
*
79+
* <ul>
80+
* <li>scala.math.BigDecimal -> java.math.BigDecimal (via underlying())
81+
* <li>scala.math.BigInt -> java.math.BigInteger (via underlying())
82+
* <li>Other types -> unchanged
83+
* </ul>
84+
*
85+
* <p>This ensures String.format receives java.math.BigDecimal/BigInteger instead of
86+
* scala.math.BigDecimal/BigInt, preventing IllegalFormatConversionException while maintaining
87+
* correct format results and taint tracking.
88+
*
89+
* @param args the array of arguments to unwrap (modified in-place)
90+
* @return the same array with ScalaNumber instances unwrapped to their Java equivalents
91+
*/
92+
@Nonnull
93+
private static Object[] unwrapScalaNumbers(@Nonnull final Object[] args) {
94+
for (int i = 0; i < args.length; i++) {
95+
if (args[i] instanceof scala.math.ScalaNumber) {
96+
args[i] = ((ScalaNumber) args[i]).underlying();
97+
}
98+
}
99+
return args;
100+
}
57101
}

‎dd-java-agent/instrumentation/scala/src/test/groovy/datadog/trace/instrumentation/scala/StringOpsFormatCallSiteTest.groovy‎

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
package datadog.trace.instrumentation.scala
2+
3+
import datadog.trace.api.iast.InstrumentationBridge
4+
import datadog.trace.api.iast.propagation.StringModule
5+
6+
/**
7+
* Tests for StringOpsCallSite.unwrapScalaNumbers functionality.
8+
*
9+
* These tests verify that Scala ScalaNumber types (BigDecimal, BigInt) are properly
10+
* unwrapped to their underlying Java representations (java.math.BigDecimal, java.math.BigInteger)
11+
* before being passed to IAST's onStringFormat.
12+
*
13+
* This prevents IllegalFormatConversionException and ensures correct taint tracking.
14+
*/
15+
class StringOpsFormatCallSiteTest extends AbstractIastScalaTest {
16+
17+
@Override
18+
String suiteName() {
19+
return 'foo.bar.TestStringOpsFormatSuite'
20+
}
21+
22+
void 'test formatBigDecimal with scala.math.BigDecimal'() {
23+
setup:
24+
final iastModule = Mock(StringModule)
25+
InstrumentationBridge.registerIastModule(iastModule)
26+
27+
when:
28+
final result = testSuite.formatBigDecimal('123.456')
29+
30+
then:
31+
result == 'Value: 123.456000'
32+
// Verify that the unwrapped java.math.BigDecimal is passed, not scala.math.BigDecimal
33+
1 * iastModule.onStringFormat(
34+
'Value: %f',
35+
{ Object[] args ->
36+
args.length == 1 &&
37+
args[0] != null &&
38+
args[0].getClass() == BigDecimal &&
39+
args[0].toString() == '123.456'
40+
},
41+
'Value: 123.456000'
42+
)
43+
0 * iastModule.onUnexpectedException(_, _)
44+
}
45+
46+
void 'test formatBigInt with scala.math.BigInt'() {
47+
setup:
48+
final iastModule = Mock(StringModule)
49+
InstrumentationBridge.registerIastModule(iastModule)
50+
51+
when:
52+
final result = testSuite.formatBigInt('12345')
53+
54+
then:
55+
result == 'Count: 12345'
56+
// Verify that the unwrapped java.math.BigInteger is passed, not scala.math.BigInt
57+
1 * iastModule.onStringFormat(
58+
'Count: %d',
59+
{ Object[] args ->
60+
args.length == 1 &&
61+
args[0] != null &&
62+
args[0].getClass() == BigInteger &&
63+
args[0].toString() == '12345'
64+
},
65+
'Count: 12345'
66+
)
67+
0 * iastModule.onUnexpectedException(_, _)
68+
}
69+
70+
void 'test formatMultipleScalaNumbers with BigDecimal and BigInt'() {
71+
setup:
72+
final iastModule = Mock(StringModule)
73+
InstrumentationBridge.registerIastModule(iastModule)
74+
75+
when:
76+
final result = testSuite.formatMultipleScalaNumbers('99.99', '42')
77+
78+
then:
79+
result == 'Decimal: 99.990000, Integer: 42'
80+
// Verify that both ScalaNumbers are unwrapped
81+
1 * iastModule.onStringFormat(
82+
'Decimal: %f, Integer: %d',
83+
{ Object[] args ->
84+
args.length == 2 &&
85+
args[0] != null &&
86+
args[0].getClass() == BigDecimal &&
87+
args[0].toString() == '99.99' &&
88+
args[1] != null &&
89+
args[1].getClass() == BigInteger &&
90+
args[1].toString() == '42'
91+
},
92+
'Decimal: 99.990000, Integer: 42'
93+
)
94+
0 * iastModule.onUnexpectedException(_, _)
95+
}
96+
97+
void 'test formatMixed with BigDecimal and String'() {
98+
setup:
99+
final iastModule = Mock(StringModule)
100+
InstrumentationBridge.registerIastModule(iastModule)
101+
102+
when:
103+
final result = testSuite.formatMixed('3.14', 'hello')
104+
105+
then:
106+
result == 'Value: 3.140000, Text: hello'
107+
// Verify that BigDecimal is unwrapped but String remains unchanged
108+
1 * iastModule.onStringFormat(
109+
'Value: %f, Text: %s',
110+
{ Object[] args ->
111+
args.length == 2 &&
112+
args[0] != null &&
113+
args[0].getClass() == BigDecimal &&
114+
args[0].toString() == '3.14' &&
115+
args[1] instanceof String &&
116+
args[1] == 'hello'
117+
},
118+
'Value: 3.140000, Text: hello'
119+
)
120+
0 * iastModule.onUnexpectedException(_, _)
121+
}
122+
123+
void 'test formatString with regular String arguments (no unwrapping)'() {
124+
setup:
125+
final iastModule = Mock(StringModule)
126+
InstrumentationBridge.registerIastModule(iastModule)
127+
128+
when:
129+
final result = testSuite.formatString('left', 'right')
130+
131+
then:
132+
result == 'Left: left, Right: right'
133+
// Verify that String arguments are passed unchanged (no unwrapping needed)
134+
1 * iastModule.onStringFormat(
135+
'Left: %s, Right: %s',
136+
{ Object[] args ->
137+
args.length == 2 &&
138+
args[0] instanceof String &&
139+
args[0] == 'left' &&
140+
args[1] instanceof String &&
141+
args[1] == 'right'
142+
},
143+
'Left: left, Right: right'
144+
)
145+
0 * iastModule.onUnexpectedException(_, _)
146+
}
147+
}

‎dd-java-agent/instrumentation/scala/src/test/scala/foo/bar/TestStringOpsFormatSuite.scala‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
package foo.bar
2+
3+
import org.slf4j.LoggerFactory
4+
5+
class TestStringOpsFormatSuite {
6+
7+
private val LOGGER = LoggerFactory.getLogger("TestStringOpsFormatSuite")
8+
9+
/** Test format with scala.math.BigDecimal using %f placeholder.
10+
*/
11+
def formatBigDecimal(value: String): String = {
12+
LOGGER.debug("Before formatBigDecimal {}", value)
13+
val bd = BigDecimal(value)
14+
val pattern = "Value: %f"
15+
val result: String = pattern.format(bd)
16+
LOGGER.debug("After formatBigDecimal {}", result)
17+
result
18+
}
19+
20+
/** Test format with scala.math.BigInt using %d placeholder.
21+
*/
22+
def formatBigInt(value: String): String = {
23+
LOGGER.debug("Before formatBigInt {}", value)
24+
val bi = BigInt(value)
25+
val pattern = "Count: %d"
26+
val result: String = pattern.format(bi)
27+
LOGGER.debug("After formatBigInt {}", result)
28+
result
29+
}
30+
31+
/** Test format with multiple ScalaNumber arguments.
32+
*/
33+
def formatMultipleScalaNumbers(decimal: String, integer: String): String = {
34+
LOGGER.debug("Before formatMultipleScalaNumbers {} {}", Array(decimal, integer): _*)
35+
val bd = BigDecimal(decimal)
36+
val bi = BigInt(integer)
37+
val pattern = "Decimal: %f, Integer: %d"
38+
val result: String = pattern.format(bd, bi)
39+
LOGGER.debug("After formatMultipleScalaNumbers {}", result)
40+
result
41+
}
42+
43+
/** Test format with mixed ScalaNumber and regular arguments.
44+
*/
45+
def formatMixed(decimal: String, text: String): String = {
46+
LOGGER.debug("Before formatMixed {} {}", Array(decimal, text): _*)
47+
val bd = BigDecimal(decimal)
48+
val pattern = "Value: %f, Text: %s"
49+
val result: String = pattern.format(bd, text)
50+
LOGGER.debug("After formatMixed {}", result)
51+
result
52+
}
53+
54+
/** Test format with regular String arguments (no unwrapping needed).
55+
*/
56+
def formatString(left: String, right: String): String = {
57+
LOGGER.debug("Before formatString {} {}", Array(left, right): _*)
58+
val pattern = "Left: %s, Right: %s"
59+
val result: String = pattern.format(left, right)
60+
LOGGER.debug("After formatString {}", result)
61+
result
62+
}
63+
}

‎dd-smoke-tests/iast-propagation/build.gradle‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ tasks.named("shadowJar", ShadowJar) {
3636
dependencies {
3737
implementation project(':dd-trace-api')
3838
implementation group: 'org.springframework.boot', name: 'spring-boot-starter-web', version: '2.5.4'
39-
implementation libs.scala
39+
implementation libs.scala213
4040
implementation libs.groovy
4141
implementation libs.kotlin
4242

‎dd-smoke-tests/iast-propagation/src/main/scala/ScalaPropagation.scala‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,4 +13,13 @@ class ScalaPropagation extends Supplier[util.List[String]] {
1313
def f(param: String): String = f"f interpolation: $param"
1414

1515
def raw(param: String): String = raw"raw interpolation: $param"
16+
17+
/** Test case to reproduce IllegalFormatConversionException from original stack trace.
18+
* Root cause: Scala's format() calls unwrapArg() which converts BigDecimal -> Double.
19+
* IAST receives BigDecimal without unwrapping, causing IllegalFormatConversionException.
20+
*/
21+
def formatBigDecimal(param: String): String = {
22+
val numericValue = BigDecimal(param)
23+
"Value: %f".format(numericValue)
24+
}
1625
}

0 commit comments

Comments
 (0)