feat: add hooks for temporal parse and string compare in equals methods#1051
feat: add hooks for temporal parse and string compare in equals methods#1051
Conversation
src/main/java/com/code_intelligence/jazzer/runtime/TraceCmpHooks.java
Outdated
Show resolved
Hide resolved
39f0285 to
cd9ecab
Compare
cd9ecab to
0ff550a
Compare
Marcono1234
left a comment
There was a problem hiding this comment.
Hopefully these comments are useful and not disruptive; if not please let me know.
| @MethodHook( | ||
| type = HookType.REPLACE, | ||
| targetClassName = "java.time.MonthDay", | ||
| targetMethod = "parse") | ||
| @MethodHook( | ||
| type = HookType.REPLACE, | ||
| targetClassName = "java.time.Duration", | ||
| targetMethod = "parse") |
There was a problem hiding this comment.
Is it intended that the annotations for MonthDay and Duration are not sorted alphabetically here?
(might make it a bit more difficult at a glance to see if all classes are covered)
| } | ||
|
|
||
| if (arguments[0] != null) { | ||
| String fallback = TEMPORAL_PARSE_FALLBACKS.get(method.type().returnType()).orElse(null); |
There was a problem hiding this comment.
(Edit: See more extensive review comment regarding this in #1051 (comment))
Out of curiosity, the above hook annotations also match the parse overloads with custom DateTimeFormatter (if I see it correctly), could that be a problem when that format completely differs from the standard ISO format?
Or is that considered acceptable, assuming that most likely the format is still somewhat close to the default ISO format?
| @@ -0,0 +1,40 @@ | |||
| /* | |||
| * Copyright 2024 Code Intelligence GmbH | |||
oetr
left a comment
There was a problem hiding this comment.
Looks nice, but there is an issue when parse has two parameters.
| @MethodHook( | ||
| type = HookType.REPLACE, | ||
| targetClassName = "java.time.Duration", | ||
| targetMethod = "parse") |
There was a problem hiding this comment.
Edit: @Marcono1234 was faster!
Concern: Some of the hooked classes have two arguments. E.g. java.time.LocalDate.parse(CharSequence text, DateTimeFormatter formatter), and since here we don't specify targetMethodDescriptor, the hooks apply to all versions of the parse function.
For the two argument version, we are guiding towards the default formatter, and not the one that the user specified.
In case of LocalDate it's ISO-8601.
It might make sense to split up the hooks to address the two overloads of parse individually.
In that case it would make sense to double-check if the user formatter is valid (it might be the reason we end up in the catch block). In that case we could safely use our default formatter.
But if the user provides a valid formatter, we should guide towards a date that respects it.
I usually abuse our fuzz test in selffuzz/src/test/java/com/code_intelligence/selffuzz/mutation/ArgumentsMutatorFuzzTest.java for quickly fuzzing anything. A fuzz test like this really struggles finding the correct string:
private static final LocalDateTime TARGET_DATE_TIME =
LocalDateTime.parse("2000-01-01T00:00:00");
@SelfFuzzTest
@Solo
void fuzzPrimitiveArrays(@NotNull String dateStr) {
try {
LocalDateTime ldt = LocalDateTime.parse(dateStr, DateTimeFormatter.ofPattern("dd MMM yyyy, HH:mm:ss", Locale.ENGLISH));
System.out.println(ldt); // this never happens
if (ldt.equals(TARGET_DATE_TIME)) {
throw new FuzzerSecurityIssueLow("Found the target date time!");
}
} catch (DateTimeParseException ignore) {
}
}To quickly run use:
bazel run //selffuzz/src/test/java/com/code_intelligence/selffuzz/mutation:ArgumentsMutatorFuzzTest -- -runs=10000000`| return Optional.of(MonthDay.from(FALLBACK_LOCAL_DATE).toString()); | ||
| if (type == ZonedDateTime.class) { | ||
| return Optional.of( | ||
| ZonedDateTime.of(FALLBACK_LOCAL_DATE, LocalTime.MIDNIGHT, ZoneId.of("Europe/Paris")) |
There was a problem hiding this comment.
Irrelevant, but fun: Opus is asking "Why Paris? Someone reading this will wonder "why Paris?" and waste 5 minutes figuring out there's no reason." 😆
Add hooks for
java.time: