Page MenuHomePhabricator

[clangd] Fix use-after-free in HeaderIncluderCache
ClosedPublic

Authored by kadircet on Oct 20 2021, 3:54 AM.

Details

Summary

Includer cache could get into a bad state when a main file went bad and
added back afterwards. This fixes the issue by getting rid of the main file to
first pointer completely to make sure we don't try invalidating again.

Diff Detail

Event Timeline

kadircet created this revision.Oct 20 2021, 3:54 AM
kadircet requested review of this revision.Oct 20 2021, 3:54 AM
kadircet updated this revision to Diff 380895.Oct 20 2021, 4:31 AM
  • Rather than dropping the entry, perform an extra check during invalidate. As

the entry actually backs the data for main file strings in associations.

sammccall accepted this revision.Oct 20 2021, 6:32 AM
sammccall added inline comments.
clang-tools-extra/clangd/TUScheduler.cpp
291

Hmm, this leaves MainToFirst pointing to an invalidated entry.
Would it make more sense to just null out the value (i.e. First here)?

Then I think we need no code changes elsewhere, as update() already checks if First already exists before invalidating.

(This special case would occur on a rarer path, but the real reason for my preference is that I think the data structure is easier to interpret)

This revision is now accepted and ready to land.Oct 20 2021, 6:32 AM
kadircet marked an inline comment as done.Oct 20 2021, 7:36 AM
kadircet added inline comments.
clang-tools-extra/clangd/TUScheduler.cpp
291

Thanks! Yeah that makes sense + now there's a point for First being a reference :P

This revision was automatically updated to reflect the committed changes.
kadircet marked an inline comment as done.