Skip to content

Commit 707c948

Browse files
simonreschsimonresch
committed
fix: fuzz test invocation with referring protobufs
When constructing a mutator for a protobuf message the mutator is cached based on the protobuf descriptor. This led to issues for fuzz tests that take two protobuf messages where one message is referring to the other. While building the mutator for the top level message nested messages are constructed with the type `DynamicMessage` which lead to 'argument type mismatch' exceptions when invoking the fuzz test method. This is fixed by including the message class in the mutator cache key.
1 parent c693437 commit 707c948

6 files changed

Lines changed: 95 additions & 38 deletions

File tree

‎selffuzz/src/test/java/com/code_intelligence/selffuzz/mutation/ArgumentsMutatorFuzzTest.java‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import com.code_intelligence.jazzer.junit.FuzzTest;
2323
import com.code_intelligence.jazzer.mutation.annotation.WithSize;
2424
import com.code_intelligence.jazzer.mutation.annotation.WithUtf8Length;
25+
import com.code_intelligence.jazzer.protobuf.Proto2;
2526
import com.code_intelligence.jazzer.protobuf.Proto3;
2627
import com.code_intelligence.selffuzz.jazzer.mutation.ArgumentsMutator;
2728
import com.code_intelligence.selffuzz.jazzer.mutation.annotation.NotNull;
@@ -246,11 +247,9 @@ void fuzz_ProtoBufsNotNull(
246247
@SelfFuzzTest
247248
void fuzz_MapField3(Proto3.MapField3 o1) {}
248249

249-
// BUG: causes java.lang.IllegalArgumentException: argument type mismatch
250-
// no problem when testing the two types separately
251-
// @SelfFuzzTest
252-
// public static void fuzz_MutuallyReferringProtobufs(
253-
// Proto2.TestProtobuf o1, Proto2.TestSubProtobuf o2) {}
250+
@SelfFuzzTest
251+
public static void fuzz_MutuallyReferringProtobufs(
252+
Proto2.TestProtobuf o1, Proto2.TestSubProtobuf o2) {}
254253

255254
/**
256255
* @return all methods in this class annotated by @SelfFuzzTest. If any of those methods are

‎src/main/java/com/code_intelligence/jazzer/mutation/mutator/proto/BuilderMutatorFactory.java‎

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -199,18 +199,15 @@ private ExtendedMutatorFactory withDescriptorDependentMutatorFactoryIfNeeded(
199199
.flatMap(
200200
clazz -> {
201201
// BuilderMutatorFactory only handles concrete subclasses of
202-
// Message.Builder
203-
// and requests Message.Builder itself for message fields, which we
204-
// handle
205-
// here.
202+
// Message.Builder and requests Message.Builder itself for message
203+
// fields, which we handle here.
206204
if (clazz != Message.Builder.class) {
207205
return Optional.empty();
208206
}
209207
// It is important that we use originalFactory here instead of factory:
210208
// factory has this field-specific message mutator appended, but this
211209
// mutator should only be used for this particular field and not any
212-
// message
213-
// subfields.
210+
// message subfields.
214211
return Optional.of(
215212
makeBuilderMutator(
216213
type,
@@ -352,7 +349,8 @@ private SerializingMutator<Any.Builder> mutatorForAny(
352349
.collect(toMap(i -> getTypeUrl(getDefaultInstance(anySource.value()[i])), identity()));
353350

354351
return assemble(
355-
mutator -> internedMutators.put(new CacheKey(Any.getDescriptor(), anySource), mutator),
352+
mutator ->
353+
internedMutators.put(new CacheKey(Any.getDescriptor(), anySource, Any.class), mutator),
356354
Any.getDefaultInstance()::toBuilder,
357355
makeBuilderSerializer(Any.getDefaultInstance()),
358356
() ->
@@ -442,7 +440,7 @@ private SerializingMutator<?> makeBuilderMutator(
442440
"@AnySource must list a non-empty list of classes");
443441
Descriptor descriptor = defaultInstance.getDescriptorForType();
444442

445-
CacheKey cacheKey = new CacheKey(descriptor, anySource);
443+
CacheKey cacheKey = new CacheKey(descriptor, anySource, defaultInstance.getClass());
446444
if (internedMutators.containsKey(cacheKey)) {
447445
return internedMutators.get(cacheKey);
448446
}
@@ -494,10 +492,13 @@ private SerializingMutator<?> makeBuilderMutator(
494492
private static final class CacheKey {
495493
private final Descriptor descriptor;
496494
private final AnySource anySource;
495+
private final Class<? extends Message> messageClass;
497496

498-
private CacheKey(Descriptor descriptor, AnySource anySource) {
497+
private CacheKey(
498+
Descriptor descriptor, AnySource anySource, Class<? extends Message> messageClass) {
499499
this.descriptor = descriptor;
500500
this.anySource = anySource;
501+
this.messageClass = messageClass;
501502
}
502503

503504
@Override
@@ -509,12 +510,16 @@ public boolean equals(Object o) {
509510
return false;
510511
}
511512
CacheKey cacheKey = (CacheKey) o;
512-
return descriptor == cacheKey.descriptor && Objects.equals(anySource, cacheKey.anySource);
513+
return descriptor == cacheKey.descriptor
514+
&& Objects.equals(anySource, cacheKey.anySource)
515+
&& Objects.equals(messageClass, cacheKey.messageClass);
513516
}
514517

515518
@Override
516519
public int hashCode() {
517-
return 31 * System.identityHashCode(descriptor) + Objects.hashCode(anySource);
520+
return 31 * System.identityHashCode(descriptor)
521+
+ Objects.hashCode(anySource)
522+
+ Objects.hashCode(messageClass);
518523
}
519524
}
520525
}

‎src/test/java/com/code_intelligence/jazzer/mutation/ArgumentsMutatorTest.java‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import com.code_intelligence.jazzer.mutation.annotation.NotNull;
2424
import com.code_intelligence.jazzer.mutation.mutator.Mutators;
2525
import com.code_intelligence.jazzer.mutation.support.TestSupport.MockPseudoRandom;
26+
import com.code_intelligence.jazzer.protobuf.Proto2;
2627
import com.code_intelligence.jazzer.protobuf.Proto2.TestProtobuf;
2728
import com.google.protobuf.DescriptorProtos;
2829
import java.io.ByteArrayInputStream;
@@ -371,4 +372,28 @@ void testProtoAndDescriptorParameters() throws NoSuchMethodException {
371372
// because "productMutator" is null
372373
assertThat(maybeMutator.toString()).contains("-> Message");
373374
}
375+
376+
// Regression: ensure that the mutator can invoke a method with two protobuf messages where one
377+
// message is a submessage of the other. This would fail with an "argument type mismatch" error
378+
// because the created Mutator for `proto2.TestProtobuf` inserted a mutator for the type
379+
// `DynamicMessage` with proto descriptor for `Proto2.TestSubProtobuf` into the
380+
// `BuilderMutatorsFactory.internedMutators` cache. This cache was then used for the second
381+
// argument in this function leading to a `DynamicMessage != Proto2.TestSubProtobuf` argument type
382+
// mismatch error.
383+
@SuppressWarnings("unused")
384+
public static void protoWithSubmessage(
385+
@NotNull Proto2.TestProtobuf proto, @NotNull Proto2.TestSubProtobuf subproto) {}
386+
387+
@Test
388+
void testProtoWithSubmessage() throws Throwable {
389+
Method method =
390+
ArgumentsMutatorTest.class.getMethod(
391+
"protoWithSubmessage", Proto2.TestProtobuf.class, Proto2.TestSubProtobuf.class);
392+
Optional<ArgumentsMutator> maybeMutator =
393+
ArgumentsMutator.forMethod(Mutators.newFactory(), method);
394+
assertThat(maybeMutator).isPresent();
395+
ArgumentsMutator mutator = maybeMutator.get();
396+
mutator.init(12345);
397+
mutator.invoke(null, false);
398+
}
374399
}

‎src/test/java/com/code_intelligence/jazzer/mutation/mutator/StressTest.java‎

Lines changed: 42 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -928,7 +928,8 @@ public static Stream<Arguments> protoStressTestCases() {
928928
OptionalPrimitiveField3.newBuilder().setSomeField(true).build())),
929929
arguments(
930930
new TypeHolder<@NotNull RepeatedRecursiveMessageField3>() {}.annotatedType(),
931-
"{Builder.Boolean, WithoutInit(Builder via List<(cycle) -> Message>)} -> Message",
931+
"{Builder.Boolean, WithoutInit(Builder via List<{Builder.Boolean, WithoutInit(Builder"
932+
+ " via List<(cycle) -> Message>)} -> Message>)} -> Message",
932933
false,
933934
// The message field is recursive and thus not initialized.
934935
exactly(
@@ -1041,14 +1042,25 @@ public static Stream<Arguments> protoStressTestCases() {
10411042
+ " Builder.Nullable<Double>, Builder.Nullable<String>,"
10421043
+ " Builder.Nullable<Enum<Enum>>,"
10431044
+ " WithoutInit(Builder.Nullable<{Builder.Nullable<Integer>, Builder via"
1044-
+ " List<Integer>, WithoutInit(Builder.Nullable<(cycle) -> Message>)} -> Message>),"
1045-
+ " Builder via List<Boolean>, Builder via List<Integer>, Builder via"
1046-
+ " List<Integer>, Builder via List<Long>, Builder via List<Long>, Builder via"
1047-
+ " List<Float>, Builder via List<Double>, Builder via List<String>, Builder via"
1048-
+ " List<Enum<Enum>>, WithoutInit(Builder via List<(cycle) -> Message>),"
1049-
+ " Builder.Map<Integer, Integer>, Builder.Nullable<FixedValue(OnlyLabel)>,"
1050-
+ " Builder.Nullable<{<empty>} -> Message>, Builder.Nullable<Integer> |"
1051-
+ " Builder.Nullable<Long> | Builder.Nullable<Integer>} -> Message",
1045+
+ " List<Integer>, WithoutInit(Builder.Nullable<{Builder.Nullable<Boolean>,"
1046+
+ " Builder.Nullable<Integer>, Builder.Nullable<Integer>, Builder.Nullable<Long>,"
1047+
+ " Builder.Nullable<Long>, Builder.Nullable<Float>, Builder.Nullable<Double>,"
1048+
+ " Builder.Nullable<String>, Builder.Nullable<Enum<Enum>>,"
1049+
+ " WithoutInit(Builder.Nullable<(cycle) -> Message>), Builder via List<Boolean>,"
1050+
+ " Builder via List<Integer>, Builder via List<Integer>, Builder via List<Long>,"
1051+
+ " Builder via List<Long>, Builder via List<Float>, Builder via List<Double>,"
1052+
+ " Builder via List<String>, Builder via List<Enum<Enum>>, WithoutInit(Builder via"
1053+
+ " List<(cycle) -> Message>), Builder.Map<Integer, Integer>,"
1054+
+ " Builder.Nullable<FixedValue(OnlyLabel)>, Builder.Nullable<{<empty>} ->"
1055+
+ " Message>, Builder.Nullable<Integer> | Builder.Nullable<Long> |"
1056+
+ " Builder.Nullable<Integer>} -> Message>)} -> Message>), Builder via"
1057+
+ " List<Boolean>, Builder via List<Integer>, Builder via List<Integer>, Builder"
1058+
+ " via List<Long>, Builder via List<Long>, Builder via List<Float>, Builder via"
1059+
+ " List<Double>, Builder via List<String>, Builder via List<Enum<Enum>>,"
1060+
+ " WithoutInit(Builder via List<(cycle) -> Message>), Builder.Map<Integer,"
1061+
+ " Integer>, Builder.Nullable<FixedValue(OnlyLabel)>, Builder.Nullable<(cycle) ->"
1062+
+ " Message>, Builder.Nullable<Integer> | Builder.Nullable<Long> |"
1063+
+ " Builder.Nullable<Integer>} -> Message",
10521064
false,
10531065
manyDistinctElements(),
10541066
manyDistinctElements()),
@@ -1063,14 +1075,25 @@ public static Stream<Arguments> protoStressTestCases() {
10631075
+ " Builder.Nullable<Double>, Builder.Nullable<String>,"
10641076
+ " Builder.Nullable<Enum<Enum>>,"
10651077
+ " WithoutInit(Builder.Nullable<{Builder.Nullable<Integer>, Builder via"
1066-
+ " List<Integer>, WithoutInit(Builder.Nullable<(cycle) -> Message>)} -> Message>),"
1067-
+ " Builder via List<Boolean>, Builder via List<Integer>, Builder via"
1068-
+ " List<Integer>, Builder via List<Long>, Builder via List<Long>, Builder via"
1069-
+ " List<Float>, Builder via List<Double>, Builder via List<String>, Builder via"
1070-
+ " List<Enum<Enum>>, WithoutInit(Builder via List<(cycle) -> Message>),"
1071-
+ " Builder.Map<Integer, Integer>, Builder.Nullable<FixedValue(OnlyLabel)>,"
1072-
+ " Builder.Nullable<{<empty>} -> Message>, Builder.Nullable<Integer> |"
1073-
+ " Builder.Nullable<Long> | Builder.Nullable<Integer>} -> Message",
1078+
+ " List<Integer>, WithoutInit(Builder.Nullable<{Builder.Nullable<Boolean>,"
1079+
+ " Builder.Nullable<Integer>, Builder.Nullable<Integer>, Builder.Nullable<Long>,"
1080+
+ " Builder.Nullable<Long>, Builder.Nullable<Float>, Builder.Nullable<Double>,"
1081+
+ " Builder.Nullable<String>, Builder.Nullable<Enum<Enum>>,"
1082+
+ " WithoutInit(Builder.Nullable<(cycle) -> Message>), Builder via List<Boolean>,"
1083+
+ " Builder via List<Integer>, Builder via List<Integer>, Builder via List<Long>,"
1084+
+ " Builder via List<Long>, Builder via List<Float>, Builder via List<Double>,"
1085+
+ " Builder via List<String>, Builder via List<Enum<Enum>>, WithoutInit(Builder via"
1086+
+ " List<(cycle) -> Message>), Builder.Map<Integer, Integer>,"
1087+
+ " Builder.Nullable<FixedValue(OnlyLabel)>, Builder.Nullable<{<empty>} ->"
1088+
+ " Message>, Builder.Nullable<Integer> | Builder.Nullable<Long> |"
1089+
+ " Builder.Nullable<Integer>} -> Message>)} -> Message>), Builder via"
1090+
+ " List<Boolean>, Builder via List<Integer>, Builder via List<Integer>, Builder"
1091+
+ " via List<Long>, Builder via List<Long>, Builder via List<Float>, Builder via"
1092+
+ " List<Double>, Builder via List<String>, Builder via List<Enum<Enum>>,"
1093+
+ " WithoutInit(Builder via List<(cycle) -> Message>), Builder.Map<Integer,"
1094+
+ " Integer>, Builder.Nullable<FixedValue(OnlyLabel)>, Builder.Nullable<(cycle) ->"
1095+
+ " Message>, Builder.Nullable<Integer> | Builder.Nullable<Long> |"
1096+
+ " Builder.Nullable<Integer>} -> Message",
10741097
false,
10751098
manyDistinctElements(),
10761099
manyDistinctElements()),
@@ -1079,8 +1102,8 @@ public static Stream<Arguments> protoStressTestCases() {
10791102
@NotNull @AnySource({PrimitiveField3.class, MessageField3.class})
10801103
AnyField3>() {}.annotatedType(),
10811104
"{Builder.Nullable<Builder.{Builder.Boolean} -> Message |"
1082-
+ " Builder.{Builder.Nullable<(cycle) -> Message>} -> Message -> Message>} ->"
1083-
+ " Message",
1105+
+ " Builder.{Builder.Nullable<{Builder.Boolean} -> Message>} -> Message ->"
1106+
+ " Message>} -> Message",
10841107
true,
10851108
exactly(
10861109
AnyField3.getDefaultInstance(),

‎src/test/java/com/code_intelligence/jazzer/mutation/mutator/proto/BuilderMutatorProto2Test.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -336,7 +336,9 @@ void testRecursiveMessageField() {
336336
factory.createInPlaceOrThrow(
337337
new TypeHolder<RecursiveMessageField2.@NotNull Builder>() {}.annotatedType());
338338
assertThat(mutator.toString())
339-
.isEqualTo("{Builder.Boolean, WithoutInit(Builder.Nullable<(cycle) -> Message>)}");
339+
.isEqualTo(
340+
"{Builder.Boolean, WithoutInit(Builder.Nullable<{Builder.Boolean,"
341+
+ " WithoutInit(Builder.Nullable<(cycle) -> Message>)} -> Message>)}");
340342
assertThat(mutator.hasFixedSize()).isFalse();
341343
RecursiveMessageField2.Builder builder = RecursiveMessageField2.newBuilder();
342344

‎src/test/java/com/code_intelligence/jazzer/mutation/mutator/proto/BuilderMutatorProto3Test.java‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -403,7 +403,9 @@ void testRecursiveMessageField() {
403403
factory.createInPlaceOrThrow(
404404
new TypeHolder<RecursiveMessageField3.@NotNull Builder>() {}.annotatedType());
405405
assertThat(mutator.toString())
406-
.isEqualTo("{Builder.Boolean, WithoutInit(Builder.Nullable<(cycle) -> Message>)}");
406+
.isEqualTo(
407+
"{Builder.Boolean, WithoutInit(Builder.Nullable<{Builder.Boolean,"
408+
+ " WithoutInit(Builder.Nullable<(cycle) -> Message>)} -> Message>)}");
407409
assertThat(mutator.hasFixedSize()).isFalse();
408410
RecursiveMessageField3.Builder builder = RecursiveMessageField3.newBuilder();
409411

@@ -609,7 +611,8 @@ void testAnyField3() throws InvalidProtocolBufferException {
609611
assertThat(mutator.toString())
610612
.isEqualTo(
611613
"{Builder.Nullable<Builder.{Builder.Boolean} -> Message |"
612-
+ " Builder.{Builder.Nullable<(cycle) -> Message>} -> Message -> Message>}");
614+
+ " Builder.{Builder.Nullable<{Builder.Boolean} -> Message>} -> Message ->"
615+
+ " Message>}");
613616
assertThat(mutator.hasFixedSize()).isTrue();
614617
AnyField3.Builder builder = AnyField3.newBuilder();
615618

0 commit comments

Comments
 (0)