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

[cmtice]

PR/167764 makes sure the access mode for newly created Native files is writable. This uncovered a bug in NativeFile::Close where it tries to flush a writable file without first checking to make sure the file hasn't already been closed. This triggers a bug in some of our code, where it closes a file by ovewriting the fields with nonsense values rather than deleting the pointer. This PR now checks to make sure this has not been done before trying to flush it.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: None (cmtice)

Changes

PR/167764 makes sure the access mode for newly created Native files is writable. This uncovered a bug in NativeFile::Close where it tries to flush a writable file without first checking to make sure the file hasn't already been closed. This triggers a bug in some of our code, where it closes a file by ovewriting the fields with nonsense values rather than deleting the pointer. This PR now checks to make sure this has not been done before trying to flush it.

---

Full diff: https://github.com/llvm/llvm-project/pull/169088.diff

1 Files Affected:

• (modified) lldb/source/Host/common/File.cpp (+4-1)

diff --git a/lldb/source/Host/common/File.cpp b/lldb/source/Host/common/File.cpp
index 4fad93fca9ea3..64504421a4d0b 100644
--- a/lldb/source/Host/common/File.cpp
+++ b/lldb/source/Host/common/File.cpp
@@ -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)) {
         if (::fflush(m_stream) == EOF)
           error = Status::FromErrno();
       }

[github-actions]

🐧 Linux x64 Test Results

• 33166 tests passed
• 491 tests skipped

[ashgti]

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?

[cmtice]

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).

[ashgti]

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

[ashgti]

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

[ashgti]

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).

[ashgti]

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.

[ashgti]

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.

[ashgti]

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

[cmtice]

It looks like this is not the correct fix for the problem, so I'll close it.