Author: John Harrison Date: 2025-11-17T10:51:13-08:00 New Revision: bb9df2e3bd7ec903f5040ec9e78bdc9e06561d67
URL: https://github.com/llvm/llvm-project/commit/bb9df2e3bd7ec903f5040ec9e78bdc9e06561d67 DIFF: https://github.com/llvm/llvm-project/commit/bb9df2e3bd7ec903f5040ec9e78bdc9e06561d67.diff LOG: [lldb] Ensure FILE* access mode is correctly specified when creating a NativeFile. (#167764) If we open a `NativeFile` with a `FILE*`, the OpenOptions default to `eOpenOptionReadOnly`. This is an issue in python scripts if you try to write to one of the files like `print("Hi", file=lldb.debugger.GetOutputFileHandle())`. To address this, we need to specify the access mode whenever we create a `NativeFile` from a `FILE*`. I also added an assert on the `NativeFile` that validates the file is opened with the correct access mode and updated `NativeFile::Read` and `NativeFile::Write` to check the access mode. Before these changes: ``` $ lldb -b -O 'script lldb.debugger.GetOutputFileHandle().write("abc")' (lldb) script lldb.debugger.GetOutputFileHandle().write("abc") Traceback (most recent call last): File "<input>", line 1, in <module> io.UnsupportedOperation: not writable ``` After: ``` $ lldb -b -O 'script lldb.debugger.GetOutputFileHandle().write("abc")' (lldb) script lldb.debugger.GetOutputFileHandle().write("abc") abc3 ``` Fixes #122387 Added: Modified: lldb/include/lldb/API/SBFile.h lldb/include/lldb/Host/File.h lldb/include/lldb/Host/StreamFile.h lldb/source/API/SBCommandReturnObject.cpp lldb/source/API/SBDebugger.cpp lldb/source/API/SBFile.cpp lldb/source/API/SBInstruction.cpp lldb/source/API/SBProcess.cpp lldb/source/API/SBStream.cpp lldb/source/Core/Debugger.cpp lldb/source/Host/common/File.cpp lldb/source/Host/common/StreamFile.cpp lldb/unittests/Host/FileTest.cpp Removed: ################################################################################ diff --git a/lldb/include/lldb/API/SBFile.h b/lldb/include/lldb/API/SBFile.h index ebdc5607b7942..8cf4fe1b405fa 100644 --- a/lldb/include/lldb/API/SBFile.h +++ b/lldb/include/lldb/API/SBFile.h @@ -27,7 +27,10 @@ class LLDB_API SBFile { SBFile(FileSP file_sp); #ifndef SWIG SBFile(const SBFile &rhs); + LLDB_DEPRECATED_FIXME("Use the constructor that specifies mode instead", + "SBFile(FILE*, const char*, bool)") SBFile(FILE *file, bool transfer_ownership); + SBFile(FILE *file, const char *mode, bool transfer_ownership); #endif SBFile(int fd, const char *mode, bool transfer_ownership); ~SBFile(); diff --git a/lldb/include/lldb/Host/File.h b/lldb/include/lldb/Host/File.h index 7402a2231735a..590c9fa523b29 100644 --- a/lldb/include/lldb/Host/File.h +++ b/lldb/include/lldb/Host/File.h @@ -66,6 +66,9 @@ class File : public IOObject { LLVM_MARK_AS_BITMASK_ENUM(/* largest_value= */ eOpenOptionInvalid) }; + static constexpr OpenOptions OpenOptionsModeMask = + eOpenOptionReadOnly | eOpenOptionWriteOnly | eOpenOptionReadWrite; + static mode_t ConvertOpenOptionsForPOSIXOpen(OpenOptions open_options); static llvm::Expected<OpenOptions> GetOptionsFromMode(llvm::StringRef mode); static bool DescriptorIsValid(int descriptor) { return descriptor >= 0; }; @@ -384,7 +387,7 @@ class NativeFile : public File { NativeFile(); - NativeFile(FILE *fh, bool transfer_ownership); + NativeFile(FILE *fh, OpenOptions options, bool transfer_ownership); NativeFile(int fd, OpenOptions options, bool transfer_ownership); diff --git a/lldb/include/lldb/Host/StreamFile.h b/lldb/include/lldb/Host/StreamFile.h index e37661a9938c0..8b01eeab6f586 100644 --- a/lldb/include/lldb/Host/StreamFile.h +++ b/lldb/include/lldb/Host/StreamFile.h @@ -81,7 +81,8 @@ class LockableStreamFile { LockableStreamFile(StreamFile &stream_file, Mutex &mutex) : m_file_sp(stream_file.GetFileSP()), m_mutex(mutex) {} LockableStreamFile(FILE *fh, bool transfer_ownership, Mutex &mutex) - : m_file_sp(std::make_shared<NativeFile>(fh, transfer_ownership)), + : m_file_sp(std::make_shared<NativeFile>(fh, File::eOpenOptionWriteOnly, + transfer_ownership)), m_mutex(mutex) {} LockableStreamFile(std::shared_ptr<File> file_sp, Mutex &mutex) : m_file_sp(file_sp), m_mutex(mutex) {} diff --git a/lldb/source/API/SBCommandReturnObject.cpp b/lldb/source/API/SBCommandReturnObject.cpp index e78e213aa23af..da7e288e38d28 100644 --- a/lldb/source/API/SBCommandReturnObject.cpp +++ b/lldb/source/API/SBCommandReturnObject.cpp @@ -15,6 +15,7 @@ #include "lldb/API/SBValue.h" #include "lldb/API/SBValueList.h" #include "lldb/Core/StructuredDataImpl.h" +#include "lldb/Host/File.h" #include "lldb/Interpreter/CommandReturnObject.h" #include "lldb/Utility/ConstString.h" #include "lldb/Utility/Instrumentation.h" @@ -275,14 +276,16 @@ void SBCommandReturnObject::SetImmediateErrorFile(FILE *fh) { void SBCommandReturnObject::SetImmediateOutputFile(FILE *fh, bool transfer_ownership) { LLDB_INSTRUMENT_VA(this, fh, transfer_ownership); - FileSP file = std::make_shared<NativeFile>(fh, transfer_ownership); + FileSP file = std::make_shared<NativeFile>(fh, File::eOpenOptionWriteOnly, + transfer_ownership); ref().SetImmediateOutputFile(file); } void SBCommandReturnObject::SetImmediateErrorFile(FILE *fh, bool transfer_ownership) { LLDB_INSTRUMENT_VA(this, fh, transfer_ownership); - FileSP file = std::make_shared<NativeFile>(fh, transfer_ownership); + FileSP file = std::make_shared<NativeFile>(fh, File::eOpenOptionWriteOnly, + transfer_ownership); ref().SetImmediateErrorFile(file); } diff --git a/lldb/source/API/SBDebugger.cpp b/lldb/source/API/SBDebugger.cpp index 5c4c653d95a81..7a4bebfdf998e 100644 --- a/lldb/source/API/SBDebugger.cpp +++ b/lldb/source/API/SBDebugger.cpp @@ -327,8 +327,8 @@ void SBDebugger::SkipAppInitFiles(bool b) { void SBDebugger::SetInputFileHandle(FILE *fh, bool transfer_ownership) { LLDB_INSTRUMENT_VA(this, fh, transfer_ownership); if (m_opaque_sp) - m_opaque_sp->SetInputFile( - (FileSP)std::make_shared<NativeFile>(fh, transfer_ownership)); + m_opaque_sp->SetInputFile((FileSP)std::make_shared<NativeFile>( + fh, File::eOpenOptionReadOnly, transfer_ownership)); } SBError SBDebugger::SetInputString(const char *data) { @@ -385,7 +385,8 @@ SBError SBDebugger::SetOutputFile(FileSP file_sp) { void SBDebugger::SetOutputFileHandle(FILE *fh, bool transfer_ownership) { LLDB_INSTRUMENT_VA(this, fh, transfer_ownership); - SetOutputFile((FileSP)std::make_shared<NativeFile>(fh, transfer_ownership)); + SetOutputFile((FileSP)std::make_shared<NativeFile>( + fh, File::eOpenOptionWriteOnly, transfer_ownership)); } SBError SBDebugger::SetOutputFile(SBFile file) { @@ -405,7 +406,8 @@ SBError SBDebugger::SetOutputFile(SBFile file) { void SBDebugger::SetErrorFileHandle(FILE *fh, bool transfer_ownership) { LLDB_INSTRUMENT_VA(this, fh, transfer_ownership); - SetErrorFile((FileSP)std::make_shared<NativeFile>(fh, transfer_ownership)); + SetErrorFile((FileSP)std::make_shared<NativeFile>( + fh, File::eOpenOptionWriteOnly, transfer_ownership)); } SBError SBDebugger::SetErrorFile(FileSP file_sp) { @@ -576,8 +578,10 @@ void SBDebugger::HandleProcessEvent(const SBProcess &process, FILE *err) { LLDB_INSTRUMENT_VA(this, process, event, out, err); - FileSP outfile = std::make_shared<NativeFile>(out, false); - FileSP errfile = std::make_shared<NativeFile>(err, false); + FileSP outfile = + std::make_shared<NativeFile>(out, File::eOpenOptionWriteOnly, false); + FileSP errfile = + std::make_shared<NativeFile>(err, File::eOpenOptionWriteOnly, false); return HandleProcessEvent(process, event, outfile, errfile); } diff --git a/lldb/source/API/SBFile.cpp b/lldb/source/API/SBFile.cpp index 2ae4b1481afbf..56909923d4b2d 100644 --- a/lldb/source/API/SBFile.cpp +++ b/lldb/source/API/SBFile.cpp @@ -39,7 +39,22 @@ SBFile::SBFile() { LLDB_INSTRUMENT_VA(this); } SBFile::SBFile(FILE *file, bool transfer_ownership) { LLDB_INSTRUMENT_VA(this, file, transfer_ownership); - m_opaque_sp = std::make_shared<NativeFile>(file, transfer_ownership); + // For backwards comptability, this defaulted to ReadOnly previously. + m_opaque_sp = std::make_shared<NativeFile>(file, File::eOpenOptionReadOnly, + transfer_ownership); +} + +SBFile::SBFile(FILE *file, const char *mode, bool transfer_ownership) { + LLDB_INSTRUMENT_VA(this, file, transfer_ownership); + + auto options = File::GetOptionsFromMode(mode); + if (!options) { + llvm::consumeError(options.takeError()); + return; + } + + m_opaque_sp = + std::make_shared<NativeFile>(file, options.get(), transfer_ownership); } SBFile::SBFile(int fd, const char *mode, bool transfer_owndership) { diff --git a/lldb/source/API/SBInstruction.cpp b/lldb/source/API/SBInstruction.cpp index 6755089af39a4..5921511f3b239 100644 --- a/lldb/source/API/SBInstruction.cpp +++ b/lldb/source/API/SBInstruction.cpp @@ -10,8 +10,8 @@ #include "lldb/Utility/Instrumentation.h" #include "lldb/API/SBAddress.h" -#include "lldb/API/SBFrame.h" #include "lldb/API/SBFile.h" +#include "lldb/API/SBFrame.h" #include "lldb/API/SBStream.h" #include "lldb/API/SBTarget.h" @@ -268,7 +268,8 @@ bool SBInstruction::GetDescription(lldb::SBStream &s) { void SBInstruction::Print(FILE *outp) { LLDB_INSTRUMENT_VA(this, outp); - FileSP out = std::make_shared<NativeFile>(outp, /*take_ownership=*/false); + FileSP out = std::make_shared<NativeFile>(outp, File::eOpenOptionWriteOnly, + /*take_ownership=*/false); Print(out); } diff --git a/lldb/source/API/SBProcess.cpp b/lldb/source/API/SBProcess.cpp index d4be64b815369..14aa9432eed83 100644 --- a/lldb/source/API/SBProcess.cpp +++ b/lldb/source/API/SBProcess.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "lldb/API/SBProcess.h" +#include "lldb/Host/File.h" #include "lldb/Utility/Instrumentation.h" #include <cinttypes> @@ -310,7 +311,8 @@ void SBProcess::ReportEventState(const SBEvent &event, SBFile out) const { void SBProcess::ReportEventState(const SBEvent &event, FILE *out) const { LLDB_INSTRUMENT_VA(this, event, out); - FileSP outfile = std::make_shared<NativeFile>(out, false); + FileSP outfile = + std::make_shared<NativeFile>(out, File::eOpenOptionWriteOnly, false); return ReportEventState(event, outfile); } diff --git a/lldb/source/API/SBStream.cpp b/lldb/source/API/SBStream.cpp index fc8f09a7bb9ae..2fc5fcfa8b0c4 100644 --- a/lldb/source/API/SBStream.cpp +++ b/lldb/source/API/SBStream.cpp @@ -116,7 +116,8 @@ void SBStream::RedirectToFile(const char *path, bool append) { void SBStream::RedirectToFileHandle(FILE *fh, bool transfer_fh_ownership) { LLDB_INSTRUMENT_VA(this, fh, transfer_fh_ownership); - FileSP file = std::make_unique<NativeFile>(fh, transfer_fh_ownership); + FileSP file = std::make_unique<NativeFile>(fh, File::eOpenOptionReadWrite, + transfer_fh_ownership); return RedirectToFile(file); } diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp index b37d9d3ed85e3..02f38e9094ec5 100644 --- a/lldb/source/Core/Debugger.cpp +++ b/lldb/source/Core/Debugger.cpp @@ -965,7 +965,8 @@ llvm::StringRef Debugger::GetStaticBroadcasterClass() { Debugger::Debugger(lldb::LogOutputCallback log_callback, void *baton) : UserID(g_unique_id++), Properties(std::make_shared<OptionValueProperties>()), - m_input_file_sp(std::make_shared<NativeFile>(stdin, NativeFile::Unowned)), + m_input_file_sp(std::make_shared<NativeFile>( + stdin, File::eOpenOptionReadOnly, NativeFile::Unowned)), m_output_stream_sp(std::make_shared<LockableStreamFile>( stdout, NativeFile::Unowned, m_output_mutex)), m_error_stream_sp(std::make_shared<LockableStreamFile>( @@ -1172,7 +1173,8 @@ Status Debugger::SetInputString(const char *data) { return result; } - SetInputFile((FileSP)std::make_shared<NativeFile>(commands_file, true)); + SetInputFile((FileSP)std::make_shared<NativeFile>( + commands_file, File::eOpenOptionReadOnly, true)); return result; } @@ -1378,7 +1380,8 @@ void Debugger::AdoptTopIOHandlerFilesIfInvalid(FileSP &in, in = GetInputFileSP(); // If there is nothing, use stdin if (!in) - in = std::make_shared<NativeFile>(stdin, NativeFile::Unowned); + in = std::make_shared<NativeFile>(stdin, File::eOpenOptionReadOnly, + NativeFile::Unowned); } // If no STDOUT has been set, then set it appropriately if (!out || !out->GetUnlockedFile().IsValid()) { diff --git a/lldb/source/Host/common/File.cpp b/lldb/source/Host/common/File.cpp index 65b75bd647c5d..4fad93fca9ea3 100644 --- a/lldb/source/Host/common/File.cpp +++ b/lldb/source/Host/common/File.cpp @@ -249,8 +249,8 @@ uint32_t File::GetPermissions(Status &error) const { NativeFile::NativeFile() = default; -NativeFile::NativeFile(FILE *fh, bool transfer_ownership) - : m_stream(fh), m_own_stream(transfer_ownership) { +NativeFile::NativeFile(FILE *fh, OpenOptions options, bool transfer_ownership) + : m_stream(fh), m_options(options), m_own_stream(transfer_ownership) { #ifdef _WIN32 // In order to properly display non ASCII characters in Windows, we need to // use Windows APIs to print to the console. This is only required if the @@ -258,6 +258,26 @@ NativeFile::NativeFile(FILE *fh, bool transfer_ownership) int fd = _fileno(fh); is_windows_console = ::GetFileType((HANDLE)::_get_osfhandle(fd)) == FILE_TYPE_CHAR; +#else +#ifndef NDEBUG + int fd = fileno(fh); + if (fd != -1) { + int required_mode = ConvertOpenOptionsForPOSIXOpen(options) & O_ACCMODE; + int mode = fcntl(fd, F_GETFL); + if (mode != -1) { + mode &= O_ACCMODE; + // Check that the file is open with a valid subset of the requested file + // access mode, e.g. if we expected the file to be writable then ensure it + // was opened with O_WRONLY or O_RDWR. + assert( + (required_mode == O_RDWR && mode == O_RDWR) || + (required_mode == O_RDONLY && (mode == O_RDWR || mode == O_RDONLY) || + (required_mode == O_WRONLY && + (mode == O_RDWR || mode == O_WRONLY))) && + "invalid file access mode"); + } + } +#endif #endif } @@ -274,7 +294,8 @@ NativeFile::NativeFile(int fd, OpenOptions options, bool transfer_ownership) } bool NativeFile::IsValid() const { - std::scoped_lock<std::mutex, std::mutex> lock(m_descriptor_mutex, m_stream_mutex); + std::scoped_lock<std::mutex, std::mutex> lock(m_descriptor_mutex, + m_stream_mutex); return DescriptorIsValidUnlocked() || StreamIsValidUnlocked(); } @@ -343,7 +364,8 @@ FILE *NativeFile::GetStream() { } Status NativeFile::Close() { - std::scoped_lock<std::mutex, std::mutex> lock(m_descriptor_mutex, m_stream_mutex); + std::scoped_lock<std::mutex, std::mutex> lock(m_descriptor_mutex, + m_stream_mutex); Status error; @@ -548,6 +570,10 @@ Status NativeFile::Sync() { Status NativeFile::Read(void *buf, size_t &num_bytes) { Status error; + // Ensure the file is open for reading. + if ((m_options & File::OpenOptionsModeMask) == eOpenOptionWriteOnly) + return Status(std::make_error_code(std::errc::bad_file_descriptor)); + #if defined(MAX_READ_SIZE) if (num_bytes > MAX_READ_SIZE) { uint8_t *p = (uint8_t *)buf; @@ -612,6 +638,10 @@ Status NativeFile::Read(void *buf, size_t &num_bytes) { Status NativeFile::Write(const void *buf, size_t &num_bytes) { Status error; + // Ensure the file is open for writing. + if ((m_options & File::OpenOptionsModeMask) == File::eOpenOptionReadOnly) + return Status(std::make_error_code(std::errc::bad_file_descriptor)); + #if defined(MAX_WRITE_SIZE) if (num_bytes > MAX_WRITE_SIZE) { const uint8_t *p = (const uint8_t *)buf; @@ -776,8 +806,8 @@ Status NativeFile::Write(const void *buf, size_t &num_bytes, off_t &offset) { int fd = GetDescriptor(); if (fd != kInvalidDescriptor) { #ifndef _WIN32 - ssize_t bytes_written = - llvm::sys::RetryAfterSignal(-1, ::pwrite, m_descriptor, buf, num_bytes, offset); + ssize_t bytes_written = llvm::sys::RetryAfterSignal( + -1, ::pwrite, m_descriptor, buf, num_bytes, offset); if (bytes_written < 0) { num_bytes = 0; error = Status::FromErrno(); diff --git a/lldb/source/Host/common/StreamFile.cpp b/lldb/source/Host/common/StreamFile.cpp index 099980a0993c6..131412d81983b 100644 --- a/lldb/source/Host/common/StreamFile.cpp +++ b/lldb/source/Host/common/StreamFile.cpp @@ -27,7 +27,8 @@ StreamFile::StreamFile(int fd, bool transfer_ownership) : Stream() { } StreamFile::StreamFile(FILE *fh, bool transfer_ownership) : Stream() { - m_file_sp = std::make_shared<NativeFile>(fh, transfer_ownership); + m_file_sp = std::make_shared<NativeFile>(fh, File::eOpenOptionWriteOnly, + transfer_ownership); } StreamFile::StreamFile(const char *path, File::OpenOptions options, diff --git a/lldb/unittests/Host/FileTest.cpp b/lldb/unittests/Host/FileTest.cpp index d973d19430596..85697c49f6fce 100644 --- a/lldb/unittests/Host/FileTest.cpp +++ b/lldb/unittests/Host/FileTest.cpp @@ -8,6 +8,7 @@ #include "lldb/Host/File.h" #include "llvm/ADT/SmallString.h" +#include "llvm/ADT/StringRef.h" #include "llvm/Support/FileSystem.h" #include "llvm/Support/FileUtilities.h" #include "llvm/Support/Path.h" @@ -35,7 +36,7 @@ TEST(File, GetWaitableHandleFileno) { FILE *stream = fdopen(fd, "r"); ASSERT_TRUE(stream); - NativeFile file(stream, true); + NativeFile file(stream, File::eOpenOptionReadWrite, true); #ifdef _WIN32 EXPECT_EQ(file.GetWaitableHandle(), (HANDLE)_get_osfhandle(fd)); #else @@ -67,3 +68,22 @@ TEST(File, GetStreamFromDescriptor) { EXPECT_EQ(file.GetWaitableHandle(), (file_t)fd); #endif } + +TEST(File, ReadOnlyModeNotWritable) { + const auto *Info = testing::UnitTest::GetInstance()->current_test_info(); + llvm::SmallString<128> name; + int fd; + llvm::sys::fs::createTemporaryFile(llvm::Twine(Info->test_case_name()) + "-" + + Info->name(), + "test", fd, name); + + llvm::FileRemover remover(name); + ASSERT_GE(fd, 0); + + NativeFile file(fd, File::eOpenOptionReadOnly, true); + ASSERT_TRUE(file.IsValid()); + llvm::StringLiteral buf = "Hello World"; + size_t bytes_written = buf.size(); + Status error = file.Write(buf.data(), bytes_written); + EXPECT_EQ(error.Fail(), true); +} _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
