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

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 2 inline comments as done.
sammccall 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();
----------------
kadircet wrote:
> 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?
Yeah fair enough - I thought having the param made the logic easier to follow but actually passing it made the code too fragile.

Just inlined badSize() and hardcoded MinSize=1, we can always extract it again later.


================
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");
----------------
kadircet wrote:
> unrelated to this patch, but it feels like we are checking for the error at the wrong place ?
agree. Will fix in a different commit


================
Comment at: clang-tools-extra/clangd/unittests/SerializationTests.cpp:320
+    if (!Succeeded)
+      elog("Failed to set rlimit");
+  }
----------------
kadircet wrote:
> should we rather `ADD_FAILURE` ?
No, because rlimit can legitimately fail (e.g. we're already using more ram than the limit we tried to set).

If rlimit fails then we may not be able to perform the test meaningfully, so we could skip it in that case (i.e. decide to *pass* immediately).


================
Comment at: clang-tools-extra/clangd/unittests/SerializationTests.cpp:345
+  std::string Serialized = llvm::to_string(Out);
+  llvm::consumeError(readIndexFile(Serialized).takeError());
+
----------------
kadircet wrote:
> why not `ASSERT_TRUE(readIndexFile(..))` ? (or `llvm::cantFail`)
Whoops, this was leftover experimentation code.


================
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);
----------------
kadircet wrote:
> can we use `In.Sources.begin()->Digest` instead?
Hmm, we could, but it has the wrong data type (uint8_t vs char) so we need a reinterpret_cast... not sure it's clearer.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91258