Skip to content

[LLDB] Make sure FILE* is valid before trying to flush it. #169088

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

Closed
wants to merge 1 commit into from
Closed

[LLDB] Make sure FILE* is valid before trying to flush it. #169088

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
5 changes: 4 additions & 1 deletion lldb/source/Host/common/File.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,10 @@ Status NativeFile::Close() {
m_options & (File::eOpenOptionReadOnly | File::eOpenOptionWriteOnly |
File::eOpenOptionReadWrite);

if (rw == eOpenOptionWriteOnly || rw == eOpenOptionReadWrite) {
// If the stream is writable, and has not already been closed, flush
// it.
if ((rw == eOpenOptionWriteOnly || rw == eOpenOptionReadWrite) &&
(m_stream->_flags != m_stream->_fileno)) {
Copy link
Contributor

@ashgti ashgti Nov 21, 2025

Choose a reason for hiding this comment

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

Isn't the FILE* type an unspecified structure on most platforms in libc? Should we be pointing into it? If its not valid, is this an issue with line 372?

Copy link
Contributor Author

@cmtice cmtice Nov 21, 2025

Choose a reason for hiding this comment

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

I am not sure; if you have a better suggestion for how to address/fix this issue I would be happy to hear it (I am not thrilled with this myself, but couldn't find a better way to do this).

Copy link
Contributor

@ashgti ashgti Nov 21, 2025

Choose a reason for hiding this comment

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

Did the test close the file somewhere else and we incorrectly assumed the file ownership transferred?

Copy link
Contributor

@ashgti ashgti Nov 21, 2025

Choose a reason for hiding this comment

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

I think maybe

llvm-project/lldb/unittests/Editline/EditlineTest.cpp

Line 50 in 427c182

const int close_result = fclose(_file_p);
is closing the file handle but somewhere we think we transferred ownership to the NativeFile

Copy link
Contributor

@ashgti ashgti Nov 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, I think I better understand the crash, its happening because ~FilePointer is closing the file, which is happening before the ~LockableStreamFile that is going to try to flush the file.

I think we need to make sure the lockable stream is destroyed before the FilePointer in the Editline tests. Or we need to open the file a second time (or dup the file).

Copy link
Contributor

@ashgti ashgti Nov 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Also, there is another issue with the test closing the file another time. The PseudoTerminal pty is not releasing the file descriptors, so it will ALSO close the file with close(fd) when its destroyed, which is the same underlying FD that the std::unique_ptr<FilePointer> _el_secondary_file is pointing to.

So, we're also closing the file out from under the libc FILE* pointer or by the time the pty object is destroyed, the file would have been closed with fclose, which should also close the fd. We're probably getting a EBADF from one of those two closes if we checked the return value.

Copy link
Contributor

@ashgti ashgti Nov 21, 2025

Choose a reason for hiding this comment

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

I think if we changed _el_secondary_file to a lldb::FileSP and use pty.ReleaseSecondaryFileDescriptor() to get the fd it should only be closed a single time. This should ensure we only close the file one time, I think.

Copy link
Contributor

@ashgti ashgti Nov 21, 2025

Choose a reason for hiding this comment

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

#169100 I think should clean up the ownership of the FDs to ensure they're only closed a single time.

if (::fflush(m_stream) == EOF)
error = Status::FromErrno();
}
Expand Down
Loading