[Lldb-commits] [lldb] bb9df2e - [lldb] Ensure FILE* access mode is correctly specified when creating a NativeFile. (#167764)
via lldb-commits
lldb-commits at lists.llvm.org
Mon Nov 17 10:51:17 PST 2025
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);
+}
More information about the lldb-commits
mailing list