Page MenuHomePhabricator

[ASTImporterTest] Fix potential use-after-free
ClosedPublic

Authored by a.sidorin on May 3 2018, 11:00 AM.

Details

Summary

buildASTFromCodeWithArgs() accepts llvm::Twine as Code argument. However, if the argument is not a C string or std::string, the argument is being copied into a temporary buffer in order to get a null-terminated string. This lead to a potential UAF. Fixing this via calling .data() on StringRef since our Code is always null-terminated.

The issue was introduced by me in D44079 (sorry) but was not noticed.

Diff Detail

Repository
rC Clang

Event Timeline

a.sidorin created this revision.May 3 2018, 11:00 AM
martong added inline comments.May 4 2018, 1:14 AM
unittests/AST/ASTImporterTest.cpp
211

Can't we have the same problem with FileName?

Perhaps an other alternative would be to make the members real strings.

std::string Code;
std::string FileName;

Hi Gabor,

Can't we have the same problem with FileName?

As I can see, no. FileName is copied into std::string while building compilation arguments.

Perhaps an other alternative would be to make the members real strings.

That was the initial solution and you was correct with it. However, it is still a workaround. The ultimate solution is to fix buildASTFromCodeWithArgs() so it can copy the buffer into a persistent storage if it is not null-terminated:

StringRef NullTerminated = Code.toNullTerminatedStringRef(CodeStorage);
auto CodeBuffer =
    CodeStorage.empty()
        ? llvm::MemoryBuffer::getMemBuffer(NullTerminated)
        : llvm::MemoryBuffer::getMemBufferCopy(NullTerminated);
InMemoryFileSystem->addFile(FileNameRef, 0, std::move(CodeBuffer));

Unfortunately, this patch has two problems. The first one is double copying of the input Twine, but it can be avoided with some additional code. The second one is copying of null-terminated StringRefs which, I guess, cannot be avoided at all because there is no legal way to check if StringRef is null-terminated or not. So, I'm summoning the initial authors of the code for the help.

@pcc, @klimek Could you propose a better idea?

a.sidorin updated this revision to Diff 146587.May 14 2018, 6:22 AM

After a number of attempts of Twine'ifying all Code samples, I decided to restore the initial state of this code.

a.sidorin updated this revision to Diff 146589.May 14 2018, 6:29 AM

Add forgotten context.

martong accepted this revision.May 14 2018, 7:57 AM

LGTM!

This revision is now accepted and ready to land.May 14 2018, 7:57 AM
xazax.hun accepted this revision.May 14 2018, 8:02 AM