This is an archive of the discontinued LLVM Phabricator instance.

[clang][Interp] Make sure we free() allocated InitMaps
ClosedPublic

Authored by tbaeder on Oct 27 2022, 2:35 AM.

Details

Summary

They get allocated when calling initialize() on a primitive array. And they get free'd when the array is fully initialized. However, when that never happens, they get leaked. Fix that by calling the destructor of global variables.

No test attached here, since the problem only exists when asan is used. However, the next patch I upload will test that InitMaps work.

Diff Detail

Event Timeline

tbaeder created this revision.Oct 27 2022, 2:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2022, 2:35 AM
tbaeder requested review of this revision.Oct 27 2022, 2:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2022, 2:35 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaron.ballman accepted this revision.Oct 27 2022, 5:41 AM

The changes look correct to me, but you may want to wait a little bit before landing to give the other reviewers a chance to look this over given that there's no tests possible for it.

This revision is now accepted and ready to land.Oct 27 2022, 5:41 AM
This revision was automatically updated to reflect the committed changes.
shafik added inline comments.Oct 28 2022, 12:08 PM
clang/lib/AST/Interp/Descriptor.cpp
46

I believe Ptr is not longer valid b/c of free(IM) b/c what Ptr points to has not been free'ed

I am looking at the wording now but I am curious what @aaron.ballman thinks.

aaron.ballman added inline comments.Oct 28 2022, 12:12 PM
clang/lib/AST/Interp/Descriptor.cpp
46

How I see it is that it's converting Ptr to an IntMap **, dereferencing that back to IntMap * and then freeing *that* pointer. So it doesn't free Ptr itself, but what Ptr points to.

shafik added inline comments.Oct 28 2022, 6:21 PM
clang/lib/AST/Interp/Descriptor.cpp
46

Right but the address is no longer valid even if the type we free'ed as is different. So the pointer is not longer pointing to a valid location. At least that is how I have seen it explained in other contexts but I have to dig up a reference.

antondaubert added inline comments.
clang/lib/AST/Interp/Program.h
54–56

Seems like this change generates a

use-of-uninitialized-value lib/AST/Interp/Descriptor.cpp:150:22 in dtorRecord(clang::interp::Block*, char*, clang::interp::Descriptor*)

when executing the test test/AST/Interp/arrays.cpp with a memory sanitizer.