Skip to content

Commit 5daf200

Browse files
authored
[BOLT] Fix memory leak in BinarySection (#82520)
The change in #80950 exposed a memory leak in BinarySection. Let BinarySection manage memory passed via updateContents() unless a valid SectionID is set indicating that the contents are managed by JITLink.
1 parent baf6bd3 commit 5daf200

File tree

4 files changed

+22
-29
lines changed

4 files changed

+22
-29
lines changed

bolt/include/bolt/Core/BinarySection.h

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -139,10 +139,7 @@ class BinarySection {
139139
Alignment = NewAlignment;
140140
ELFType = NewELFType;
141141
ELFFlags = NewELFFlags;
142-
OutputSize = NewSize;
143-
OutputContents = StringRef(reinterpret_cast<const char *>(NewData),
144-
NewData ? NewSize : 0);
145-
IsFinalized = true;
142+
updateContents(NewData, NewSize);
146143
}
147144

148145
public:
@@ -484,9 +481,18 @@ class BinarySection {
484481
void flushPendingRelocations(raw_pwrite_stream &OS,
485482
SymbolResolverFuncTy Resolver);
486483

487-
/// Change contents of the section.
488-
void updateContents(const uint8_t *Data, size_t NewSize) {
489-
OutputContents = StringRef(reinterpret_cast<const char *>(Data), NewSize);
484+
/// Change contents of the section. Unless the section has a valid SectionID,
485+
/// the memory passed in \p NewData will be managed by the instance of
486+
/// BinarySection.
487+
void updateContents(const uint8_t *NewData, size_t NewSize) {
488+
if (getOutputData() && !hasValidSectionID() &&
489+
(!hasSectionRef() ||
490+
OutputContents.data() != getContentsOrQuit(Section).data())) {
491+
delete[] getOutputData();
492+
}
493+
494+
OutputContents = StringRef(reinterpret_cast<const char *>(NewData),
495+
NewData ? NewSize : 0);
490496
OutputSize = NewSize;
491497
IsFinalized = true;
492498
}

bolt/lib/Core/BinarySection.cpp

Lines changed: 1 addition & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -190,18 +190,7 @@ void BinarySection::flushPendingRelocations(raw_pwrite_stream &OS,
190190
clearList(PendingRelocations);
191191
}
192192

193-
BinarySection::~BinarySection() {
194-
if (isReordered()) {
195-
delete[] getData();
196-
return;
197-
}
198-
199-
if (!isAllocatable() && !hasValidSectionID() &&
200-
(!hasSectionRef() ||
201-
OutputContents.data() != getContentsOrQuit(Section).data())) {
202-
delete[] getOutputData();
203-
}
204-
}
193+
BinarySection::~BinarySection() { updateContents(nullptr, 0); }
205194

206195
void BinarySection::clearRelocations() { clearList(Relocations); }
207196

bolt/lib/Rewrite/RewriteInstance.cpp

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4092,12 +4092,9 @@ void RewriteInstance::rewriteNoteSections() {
40924092
return getNewValueForSymbol(S->getName());
40934093
});
40944094

4095-
// Set/modify section info.
4096-
BinarySection &NewSection = BC->registerOrUpdateNoteSection(
4097-
SectionName, SectionData, Size, Section.sh_addralign,
4098-
!BSec->isWritable(), BSec->getELFType());
4099-
NewSection.setOutputAddress(0);
4100-
NewSection.setOutputFileOffset(NextAvailableOffset);
4095+
// Section contents are no longer needed, but we need to update the size so
4096+
// that it will be reflected in the section header table.
4097+
BSec->updateContents(nullptr, Size);
41014098

41024099
NextAvailableOffset += Size;
41034100
}

bolt/unittests/Core/BinaryContext.cpp

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -77,10 +77,11 @@ TEST_P(BinaryContextTester, FlushPendingRelocCALL26) {
7777
// 12: bl func2
7878
// 16: func2
7979

80-
char Data[20] = {};
80+
constexpr size_t DataSize = 20;
81+
uint8_t *Data = new uint8_t[DataSize];
8182
BinarySection &BS = BC->registerOrUpdateSection(
82-
".text", ELF::SHT_PROGBITS, ELF::SHF_EXECINSTR | ELF::SHF_ALLOC,
83-
(uint8_t *)Data, sizeof(Data), 4);
83+
".text", ELF::SHT_PROGBITS, ELF::SHF_EXECINSTR | ELF::SHF_ALLOC, Data,
84+
DataSize, 4);
8485
MCSymbol *RelSymbol1 = BC->getOrCreateGlobalSymbol(4, "Func1");
8586
ASSERT_TRUE(RelSymbol1);
8687
BS.addRelocation(8, RelSymbol1, ELF::R_AARCH64_CALL26, 0, 0, true);
@@ -89,7 +90,7 @@ TEST_P(BinaryContextTester, FlushPendingRelocCALL26) {
8990
BS.addRelocation(12, RelSymbol2, ELF::R_AARCH64_CALL26, 0, 0, true);
9091

9192
std::error_code EC;
92-
SmallVector<char> Vect(sizeof(Data));
93+
SmallVector<char> Vect(DataSize);
9394
raw_svector_ostream OS(Vect);
9495

9596
BS.flushPendingRelocations(OS, [&](const MCSymbol *S) {

0 commit comments

Comments
 (0)