[libfuzzer] add test of cov file-id in control file

[yingcong-wu]

Summary

There is test for ft file-id in control file, but no test for cov line.
Without the test, a invalid cov file-id would cause crash.

[yingcong-wu]

add a comment to test

[yingcong-wu]

Hi all, kindly ping.

[vitalybuka]

compiler-rt/lib/fuzzer/FuzzerMerge.cpp

113

this one is LGTM, and I see the test

123

but i am not sure what this condition for, and I guess it's not covered by test?
maybe it would be easier to understand with a test.

[yingcong-wu]

compiler-rt/lib/fuzzer/FuzzerMerge.cpp

123

From the test I see that FT marker is required, but COV marker is optional. In the original code, LastSeenStartMarker also serves as a flag to mark if the FT marker is presented (by setting it to kInvalidStartMarker). However, because we want to keep LastSeenStartMarker's value to check with COV marker, so I add the HaveFtMarker as a flag to see if the FT marker is presented or not.

[yingcong-wu]

• set init value of HaveFtMarker to ture, for it's ok to not have any STARTED

[vitalybuka]

compiler-rt/lib/fuzzer/FuzzerMerge.cpp

123–126

I believe your code is equivalent to

if (!HaveFtMarker && LastSeenStartMarker != kInvalidStartMarker)
  LastFailure = Files[LastSeenStartMarker].Name;

But as I see, the only way to have HaveFtMarker == false is to go through (Marker == "STARTED") so LastSeenStartMarker will be set.
So

LastFailure = Files[LastSeenStartMarker].Name;

will never happen

Would you like to extract HaveFtMarker change into a separate patch, with own test?

[yingcong-wu]

compiler-rt/lib/fuzzer/FuzzerMerge.cpp

123–126

I don't see why

LastFailure = Files[LastSeenStartMarker].Name;

will never happen.

When going through (Marker == "STARTED"), HaveFtMarker is set to false and LastSeenStartMarker will have a meaningful value, assuming without going through (Marker == "FT"), then (!HaveFtMarker && LastSeenStartMarker != kInvalidStartMarker) is true and LastFailure = Files[LastSeenStartMarker].Name; will happen.

But I see that the condition could be simplified as you suggested.

Also, I don't think it's a good idea to move HaveFtMarker change to another patch because without HaveFtMarker, this patch will break some LastFailure assertions in fuzzer unittest.

[yingcong-wu]

compiler-rt/lib/fuzzer/FuzzerMerge.cpp

123–126

Also, it is difficult to write a test for HaveFtMarker, bacause:

1. it does not introduce new behavior
2. merge control with FT maker is ok, so it cannot be test with lit
3. there are already unittest to test for LastFailure with merge control file which have STARTED but no FT

[yingcong-wu]

simplify the LastFailure condition

[vitalybuka]

compiler-rt/lib/fuzzer/FuzzerMerge.cpp

123–126

I don't see why

LastFailure = Files[LastSeenStartMarker].Name;

will never happen.

Thanks, you are correct.

When going through (Marker == "STARTED"), HaveFtMarker is set to false and LastSeenStartMarker will have a meaningful value, assuming without going through (Marker == "FT"), then (!HaveFtMarker && LastSeenStartMarker != kInvalidStartMarker) is true and LastFailure = Files[LastSeenStartMarker].Name; will happen.

But I see that the condition could be simplified as you suggested.

Also, I don't think it's a good idea to move HaveFtMarker change to another patch because without HaveFtMarker, this patch will break some LastFailure assertions in fuzzer unittest.

If this fixes some assertions then we can count that as a test coverage.

[yingcong-wu]

Thank you. I will remember to mention if something is added to fix testes.

compiler-rt/lib/fuzzer/FuzzerMerge.cpp

bool Merger::Parse(std::istream &IS, bool ParseCoverage) {
   for (size_t i = 0; i < NumFiles; i++)
     if (!std::getline(IS, Files[i].Name, '\n'))
       return false;
 
   // Parse STARTED, FT, and COV lines.
   size_t ExpectedStartMarker = 0;
   const size_t kInvalidStartMarker = -1;
   size_t LastSeenStartMarker = kInvalidStartMarker;
+  bool HaveFtMarker = true;
   std::vector<uint32_t> TmpFeatures;
   std::set<uint32_t> PCs;
   while (std::getline(IS, Line, '\n')) {
     std::istringstream ISS1(Line);
     std::string Marker;
     uint32_t N;
     if (!(ISS1 >> Marker) || !(ISS1 >> N))
       return false;
     if (Marker == "STARTED") {
       // STARTED FILE_ID FILE_SIZE
       if (ExpectedStartMarker != N)
         return false;
       ISS1 >> Files[ExpectedStartMarker].Size;
       LastSeenStartMarker = ExpectedStartMarker;
       assert(ExpectedStartMarker < Files.size());
       ExpectedStartMarker++;
+      HaveFtMarker = false;
     } else if (Marker == "FT") {
       // FT FILE_ID COV1 COV2 COV3 ...
       size_t CurrentFileIdx = N;
       if (CurrentFileIdx != LastSeenStartMarker)
         return false;
-      LastSeenStartMarker = kInvalidStartMarker;
+      HaveFtMarker = true;
       if (ParseCoverage) {
         TmpFeatures.clear();  // use a vector from outer scope to avoid resizes.
         while (ISS1 >> N)
           TmpFeatures.push_back(N);
         std::sort(TmpFeatures.begin(), TmpFeatures.end());
         Files[CurrentFileIdx].Features = TmpFeatures;
       }
     } else if (Marker == "COV") {
       size_t CurrentFileIdx = N;
+      if (CurrentFileIdx != LastSeenStartMarker)
+        return false;
       if (ParseCoverage)
         while (ISS1 >> N)
           if (PCs.insert(N).second)
             Files[CurrentFileIdx].Cov.push_back(N);
     } else {
       return false;
     }
   }
-  if (LastSeenStartMarker != kInvalidStartMarker)
+  if (!HaveFtMarker && LastSeenStartMarker != kInvalidStartMarker)
     LastFailure = Files[LastSeenStartMarker].Name;
 
   FirstNotProcessedFile = ExpectedStartMarker;
   return true;
 }
 
 size_t Merger::ApproximateMemoryConsumption() const  {
   size_t Res = 0;
   for (const auto &F: Files)
     Res += sizeof(F) + F.Features.size() * sizeof(F.Features[0]);
   return Res;

compiler-rt/test/fuzzer/merge-control-file.test

 RUN: echo STARTED 0 1 >> %t/MCF
 RUN: echo FT 0 11 >> %t/MCF
 RUN: echo STARTED 1 2 >> %t/MCF
 RUN: echo FT 1 12 >> %t/MCF
 RUN: echo STARTED 2 2 >> %t/MCF
 RUN: echo FT 2 13 >> %t/MCF
 RUN: %run %t/T.exe -merge=1 %t/T1 %t/T2 -merge_control_file=%t/MCF 2>&1 | FileCheck %s --check-prefix=OK_3
 OK_3: MERGE-OUTER: nothing to do, merge has been completed before
+
+# Test for invalid COV file_id
+RUN: rm -f %t/T1/*; cp %t/T0/* %t/T1
+RUN: echo 3 > %t/MCF; echo 0 >> %t/MCF; echo %t/T1/1 >> %t/MCF; echo %t/T1/2 >> %t/MCF; echo %t/T1/3 >> %t/MCF
+RUN: echo STARTED 0 1 >> %t/MCF
+RUN: echo FT 0 11 >> %t/MCF
+RUN: echo STARTED 1 2 >> %t/MCF
+RUN: echo FT 1 12 >> %t/MCF
+RUN: echo STARTED 2 2 >> %t/MCF
+RUN: echo FT 2 13 >> %t/MCF
+# Invalid file-id 21 here
+RUN: echo COV 21 13 >> %t/MCF
+RUN: %run %t/T.exe -merge=1 %t/T1 %t/T2 -merge_control_file=%t/MCF 2>&1 | FileCheck %s --check-prefix=COV_INVALID
+COV_INVALID: MERGE-OUTER: bad control file, will overwrite it