Skip to content

Commit

Permalink
Fix FILE * leak in Python API
Browse files Browse the repository at this point in the history
Summary:
This fixes a leak introduced by some of these changes:
r257644
r250530
r250525

The changes made in these patches result in leaking the FILE* passed
to SetImmediateOutputFile. GetStream() will dup() the fd held by the
python caller and create a new FILE*. It will then pass this FILE*
to SetImmediateOutputFile, which always uses the flag
transfer_ownership=false when it creates a File from the FILE*.

Since transfer_ownership is false, the lldb File destructor will not
close the underlying FILE*. Because this FILE* came from a dup-ed fd,
it will also not be closed when the python caller closes its file.

Leaking the FILE* causes issues if the same file is used multiple times
by different python callers during the same lldb run, even if these
callers open and close the python file properly, as you can end up
with issues due to multiple buffered writes to the same file.

Reviewers: granata.enrico, zturner, clayborg

Subscribers: zturner, lldb-commits, sas

Differential Revision: http://reviews.llvm.org/D18459

Change by Francis Ricci <fjricci@fb.com>

llvm-svn: 270066
  • Loading branch information
fjricci committed May 19, 2016
1 parent 2f32cae commit d1d4798
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 9 deletions.
10 changes: 9 additions & 1 deletion lldb/include/lldb/API/SBCommandReturnObject.h
Expand Up @@ -83,13 +83,21 @@ class LLDB_API SBCommandReturnObject

bool
GetDescription (lldb::SBStream &description);


// deprecated, these two functions do not take
// ownership of file handle
void
SetImmediateOutputFile (FILE *fh);

void
SetImmediateErrorFile (FILE *fh);

void
SetImmediateOutputFile (FILE *fh, bool transfer_ownership);

void
SetImmediateErrorFile (FILE *fh, bool transfer_ownership);

void
PutCString(const char* string, int len = -1);

Expand Down
16 changes: 11 additions & 5 deletions lldb/scripts/interface/SBCommandReturnObject.i
Expand Up @@ -84,11 +84,17 @@ public:
bool
GetDescription (lldb::SBStream &description);

void
SetImmediateOutputFile (FILE *fh);

void
SetImmediateErrorFile (FILE *fh);

// wrapping here so that lldb takes ownership of the
// new FILE* created inside of the swig interface
%extend {
void SetImmediateOutputFile(FILE *fh) {
self->SetImmediateOutputFile(fh, true);
}
void SetImmediateErrorFile(FILE *fh) {
self->SetImmediateErrorFile(fh, true);
}
}

void
PutCString(const char* string, int len);
Expand Down
18 changes: 15 additions & 3 deletions lldb/source/API/SBCommandReturnObject.cpp
Expand Up @@ -258,15 +258,27 @@ SBCommandReturnObject::GetDescription (SBStream &description)
void
SBCommandReturnObject::SetImmediateOutputFile(FILE *fh)
{
if (m_opaque_ap)
m_opaque_ap->SetImmediateOutputFile(fh);
SetImmediateOutputFile(fh, false);
}

void
SBCommandReturnObject::SetImmediateErrorFile(FILE *fh)
{
SetImmediateErrorFile(fh, false);
}

void
SBCommandReturnObject::SetImmediateOutputFile(FILE *fh, bool transfer_ownership)
{
if (m_opaque_ap)
m_opaque_ap->SetImmediateOutputFile(fh, transfer_ownership);
}

void
SBCommandReturnObject::SetImmediateErrorFile(FILE *fh, bool transfer_ownership)
{
if (m_opaque_ap)
m_opaque_ap->SetImmediateErrorFile(fh);
m_opaque_ap->SetImmediateErrorFile(fh, transfer_ownership);
}

void
Expand Down

0 comments on commit d1d4798

Please sign in to comment.