Skip to content

Memory leak fix in SPIRVFunction::decodeBB #2268

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
MrSidims merged 1 commit into from
Jan 8, 2024
Merged

Memory leak fix in SPIRVFunction::decodeBB #2268

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/SPIRV/libSPIRV/SPIRVFunction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ bool SPIRVFunction::decodeBB(SPIRVDecoder &Decoder) {
std::to_string(Entry->getOpCode()))) {
// Bail out if the opcode is not implemented.
Module->setInvalid();
delete Entry;
Copy link
Contributor

@asudarsa asudarsa Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @bwlodarcz

Can you please recheck this? I see that we eventually call into some code that returns an entry from a SPIRVFactory. Not sure if we can delete such pointers here.

Thanks

Copy link
Contributor

@MrSidims MrSidims Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move delete to Decoder d'tor eliminating all Entries from the map (or whatever container we use underneath getDecoder), or am I missing something?

Copy link
Contributor Author

@bwlodarcz bwlodarcz Dec 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First @asudarsa:

Can you please recheck this? I see that we eventually call into some code that returns an entry from a SPIRVFactory. Not sure if we can delete such pointers here.

Allocation of SPIRVEntry starts in template <typename T> SPIRVEntry *create() { return new T(); } on lib/SPIRV/libSPIRV/SPIRVEntry.cpp:62 - raw new/malloc.
As we can see in the reviewed function above we have Decoder.getEntry() call which points out to
SPIRVEntry *SPIRVDecoder::getEntry method located in lib/SPIRV/libSPIRV/SPIRVStream.cpp:239.
Overall we can see that it's some kind of factory method which initializes Entry.
During this initialization we have call to Entry->setModule(&M) where Entry points out to Module - but not the other way.

void SPIRVEntry::setModule(SPIRVModule *TheModule) {
  assert(TheModule && "Invalid module");
  if (TheModule == Module)
    return;
  assert(Module == NULL && "Cannot change owner of entry");
  Module = TheModule;
}

So right only the entry is aware of Module not vice versa.
And after that we are in our SPIRVFunction::decodeBB. From that point If the SPIRVEntry pointer is lost by some condition, before it get's assigned to SPIRVModule instance which happens later, we are unable to recover it by any means. That's why the delete/free is a necessity in such branches.
I rechecked and when the deletes are not in place than it leaks again.

Second @MrSidims:

Can we move delete to Decoder d'tor eliminating all Entries from the map (or whatever container we use underneath getDecoder), or am I missing something?

It seems that, in theory, in Decoder/Encoder iostream architecture SPIRVModule owns SPIRVEntries and it's responsible for the destruction - look on SPIRVModuleImpl destructor. In reality, SPIRVEntries are created outside of lifetime scope of SPIRVModule and eventually passed to it when we are coming through happy path. There are paths/branches where SPIRVEntry is created but doesn't fit (like in this PR) - in these cases SPIRVModule isn't an owner of the resource and it needs to be manually freed.
Changing an architecture of Decoder/Encoder is much more work and outside of scope of fixing direct ASan errors. I think that we, as community, should definitely think about such architectural change in future.

return false;
}

Expand All @@ -164,6 +165,7 @@ bool SPIRVFunction::decodeBB(SPIRVDecoder &Decoder) {
SPIRVDebug::DebugNoLine) ||
Inst->isExtInst(SPIRVEIS_NonSemantic_Shader_DebugInfo_200,
SPIRVDebug::DebugNoLine)) {
delete Entry;
Copy link
Contributor

@asudarsa asudarsa Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above

continue;
} else if (Inst->isExtInst(SPIRVEIS_NonSemantic_Shader_DebugInfo_100,
SPIRVDebug::DebugLine) ||
Expand Down