[llvm][ProfileData] Add a warning to indicate potentially corrupted data

[leonardchan]

Summary

This is meant to address PR51628 where we hit an OOM error when accidentally uncompressing corrupted raw profile data. Ideally, there would be some warning or indicator of why this occurred rather than just failing with OOM.

[leonardchan]

I'm not particularly fond of this solution, but I couldn't think of anything better (and am open for suggestions). My initial thought was that we could instead use a char * for the buffer rather than a SmallString, and when allocating the buffer we could just check if the malloc for the UncompressedSize failed or not (nullptr could indicate either corrupt value or just not enough memory). But we'd still run into the LLVM ERROR on the malloc since LLVM would still catch the error (via signal handlers) and do the normal error reporting.

Additionally, I couldn't find other ways we could "validate" the uncompressed size. That is, I don't *think* there is any absolute way of determining if the uncompressed size is corrupted data or the uncompressed size is just really large.

[dblaikie]

Is OOM worth avoiding? Presumably some real-world cases could exceed the limits of a given machine anyway, right? (I'm not sure that LLVM and Clang avoid OOM, for instance - you can probably make some Clang AST that can't be stored in the memory available on some systems)

[davidxl]

I think this warning is not about OOM, but corrupted profile data. Regular OOM may not worth warning.

[dblaikie]

What's the nature of the corruption?
I'd probably prefer to avoid heuristics like the one suggested here. Is there something in the uncompressed data that is certainly wrong in this case?

[davidxl]

It seems that the uncompressed size field is corrupted and becomes too large. The warning seems just catching this quite narrow case.

[dblaikie]

The definition of "too large" seems heuristical though - it's not provably wrong, just unlikely/'weird'. It's possible a legitimate use could have such a value - and the right answer would be to OOM, basically?

[dblaikie]

I guess I don't mind it too much as a warning, at least. Doesn't block it from working if there's a legitimate input that happens to compress well.

[mcgrathr]

The intent is to sanity-check for garbage header fields. The uncompressed size field is one that you can only warn about heuristically like this I guess. But you can do a definitive test on the CompressedSize field to ensure that it isn't larger than the actual size of the data available. There's no need to even worry about whether UncompressedSize is valid if CompressedSize is clearly invalid.

[phosek]

Since these fields come from the binary and should be identical (profile runtime simply copies them into the raw profile), a different approach would be to have a new subcommand in llvm-profdata to validate the profile against the binary to check if those fields are indeed the same, for example something like llvm-profdata validate --object=<binary> <profile>.profraw (alternatively it could also be just a flag, for example llvm-profdata merge --validate=<binary> <profile>.profraw).

[davidxl]

Both Roland and Petr's suggestions are great.

llvm/lib/ProfileData/InstrProf.cpp

while (P < EndP) {
     P += N;
     bool isCompressed = (CompressedSize != 0);
     SmallString<128> UncompressedNameStrings;
     StringRef NameStrings;
     if (isCompressed) {
       if (!llvm::zlib::isAvailable())
         return make_error<InstrProfError>(instrprof_error::zlib_unavailable);
 
+      // Sanity-check the uncompressed size. If the data is corrupted, then we
+      // could run into a situation where we attempt to malloc a large amount of
+      // data and OOM (see PR51628). While we technically do not have a way to
+      // error-check if the size is actually "correct", we can at least guess
+      // beforehand if the data is corrupt by comparing the uncompressed size to
+      // the compressed size. If the ratio is large enough, then we can warn.
+      //
+      // https://zlib.net/zlib_tech.html stats that typical zlib compression
+      // factors are in the range of 2:1 and 5:1. The value used here should be
+      // large enough such that we minimize the number of "false positives" from
+      // decompressions that we can properly allocate for.
+      constexpr size_t CompressionRatio = 10;
+      if (UncompressedSize > CompressedSize * CompressionRatio) {
+        llvm::errs()
+            << "warning: the uncompressed size is more than "
+            << CompressionRatio
+            << "x the size of the compressed size, which could indicate data "
+               "corruption in the uncompressed size and lead to an OOM error\n";
+      }
+
       StringRef CompressedNameStrings(reinterpret_cast<const char *>(P),
                                       CompressedSize);
       if (Error E =
               zlib::uncompress(CompressedNameStrings, UncompressedNameStrings,
                                UncompressedSize)) {
         consumeError(std::move(E));
         return make_error<InstrProfError>(instrprof_error::uncompress_failed);
       }