[PATCH] D91258: [clangd] Sanity-check array sizes read from disk before allocating them.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/index/Serialization.cpp:121
+  template <typename T>
+  LLVM_NODISCARD bool consumeSize(T &Container, unsigned MinSize = 1) {
+    auto Size = consumeVar();
----------------
regarding minsizes, i suppose the idea was to pass `ElementSizeInBytes` for containers ? I am OK with the overshooting if you don't want to make this more detailed, but can we drop the param?


================
Comment at: clang-tools-extra/clangd/index/Serialization.cpp:219
   Reader R(Data);
   size_t UncompressedSize = R.consume32();
   if (R.err())
----------------
in theory, this could also trip us over. zlib::uncompress does a `reserve` using this value. it is harder to come up with an upper bound here, but https://zlib.net/zlib_tech.html#:~:text=Maximum%20Compression%20Factor&text=(The%20test%20case%20was%20a,sources)%20is%201032%3A1. says the theoretical limit is 1032:1.
Maybe enforce that?

Also to make this more similar to others, it looks like `zlib::uncompress` also has an overload taking a `const char *`.


================
Comment at: clang-tools-extra/clangd/index/Serialization.cpp:529
     Reader CmdReader(Chunks.lookup("cmdl"));
     if (CmdReader.err())
       return error("malformed or truncated commandline section");
----------------
unrelated to this patch, but it feels like we are checking for the error at the wrong place ?


================
Comment at: clang-tools-extra/clangd/unittests/SerializationTests.cpp:320
+    if (!Succeeded)
+      elog("Failed to set rlimit");
+  }
----------------
should we rather `ADD_FAILURE` ?


================
Comment at: clang-tools-extra/clangd/unittests/SerializationTests.cpp:345
+  std::string Serialized = llvm::to_string(Out);
+  llvm::consumeError(readIndexFile(Serialized).takeError());
+
----------------
why not `ASSERT_TRUE(readIndexFile(..))` ? (or `llvm::cantFail`)


================
Comment at: clang-tools-extra/clangd/unittests/SerializationTests.cpp:364
+  // The offset isn't trivial to find, so we use the file digest.
+  std::string FileDigest = llvm::fromHex("EED8F5EAF25C453C");
+  unsigned Pos = Srcs->Data.find_first_of(FileDigest);
----------------
can we use `In.Sources.begin()->Digest` instead?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91258/new/

https://reviews.llvm.org/D91258