Skip to content

feat: add hooks for temporal parse and string compare in equals methods#1051

Open
onionpsy wants to merge 1 commit intomainfrom
cif-1920-time-hooks
Open

feat: add hooks for temporal parse and string compare in equals methods#1051
onionpsy wants to merge 1 commit intomainfrom
cif-1920-time-hooks

Conversation

@onionpsy
Copy link
Contributor

Add hooks for java.time:

  • add guidance for equality checks
  • adds parsing hooks so failed time/date parses still provide useful guidance to the fuzzer

@onionpsy onionpsy force-pushed the cif-1920-time-hooks branch from 39f0285 to cd9ecab Compare March 12, 2026 09:04
@onionpsy onionpsy force-pushed the cif-1920-time-hooks branch from cd9ecab to 0ff550a Compare March 12, 2026 09:32
@onionpsy onionpsy marked this pull request as ready for review March 12, 2026 10:44
Copy link
Contributor

@Marcono1234 Marcono1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully these comments are useful and not disruptive; if not please let me know.

Comment on lines +393 to +400
@MethodHook(
type = HookType.REPLACE,
targetClassName = "java.time.MonthDay",
targetMethod = "parse")
@MethodHook(
type = HookType.REPLACE,
targetClassName = "java.time.Duration",
targetMethod = "parse")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Contributor

@Marcono1234 Marcono1234 Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be 2026?

Copy link
Contributor

@oetr oetr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice, but there is an issue when parse has two parameters.

@MethodHook(
type = HookType.REPLACE,
targetClassName = "java.time.Duration",
targetMethod = "parse")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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." 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants