-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[lldb] Eliminate SupportFileSP nullptr derefs #168624
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.
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
base: main
Are you sure you want to change the base?
Conversation
This patch fixes and eliminates the possibility of SupportFileSP ever being nullptr. The support file was originally treated like a value type, but became a polymorphic type and therefore has to be stored and passed around as a pointer. To avoid having all the callers check the validity of the pointer, I introduced the invariant that SupportFileSP is never null and always default constructed. However, without enforcement at the type level, that's fragile and indeed, we already identified two crashes where someone accidentally broke that invariant. This PR introduces a NonNullSharedPtr to prevent that. NonNullSharedPtr is a smart pointer wrapper around std::shared_ptr that guarantees the pointer is never null. If default-constructed, it creates a default-constructed instance of the contained type. rdar://164989579
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesThis patch fixes and eliminates the possibility of SupportFileSP ever being nullptr. The support file was originally treated like a value type, but became a polymorphic type and therefore has to be stored and passed around as a pointer. To avoid having all the callers check the validity of the pointer, I introduced the invariant that SupportFileSP is never null and always default constructed. However, without enforcement at the type level, that's fragile and indeed, we already identified two crashes where someone accidentally broke that invariant. This PR introduces a NonNullSharedPtr to prevent that. NonNullSharedPtr is a smart pointer wrapper around std::shared_ptr that guarantees the pointer is never null. If default-constructed, it creates a default-constructed instance of the contained type. rdar://164989579 Full diff: https://github.com/llvm/llvm-project/pull/168624.diff 14 Files Affected:
diff --git a/lldb/include/lldb/Core/SourceManager.h b/lldb/include/lldb/Core/SourceManager.h
index 83dc74768733d..f2b08528b6854 100644
--- a/lldb/include/lldb/Core/SourceManager.h
+++ b/lldb/include/lldb/Core/SourceManager.h
@@ -11,6 +11,7 @@
#include "lldb/Utility/Checksum.h"
#include "lldb/Utility/FileSpec.h"
+#include "lldb/Utility/SupportFile.h"
#include "lldb/lldb-defines.h"
#include "lldb/lldb-forward.h"
@@ -38,8 +39,8 @@ class SourceManager {
const SourceManager::File &rhs);
public:
- File(lldb::SupportFileSP support_file_sp, lldb::TargetSP target_sp);
- File(lldb::SupportFileSP support_file_sp, lldb::DebuggerSP debugger_sp);
+ File(SupportFileSP support_file_sp, lldb::TargetSP target_sp);
+ File(SupportFileSP support_file_sp, lldb::DebuggerSP debugger_sp);
bool ModificationTimeIsStale() const;
bool PathRemappingIsStale() const;
@@ -57,7 +58,7 @@ class SourceManager {
bool LineIsValid(uint32_t line);
- lldb::SupportFileSP GetSupportFile() const {
+ SupportFileSP GetSupportFile() const {
assert(m_support_file_sp && "SupportFileSP must always be valid");
return m_support_file_sp;
}
@@ -80,13 +81,13 @@ class SourceManager {
protected:
/// Set file and update modification time.
- void SetSupportFile(lldb::SupportFileSP support_file_sp);
+ void SetSupportFile(SupportFileSP support_file_sp);
bool CalculateLineOffsets(uint32_t line = UINT32_MAX);
/// The support file. If the target has source mappings, this might be
/// different from the original support file passed to the constructor.
- lldb::SupportFileSP m_support_file_sp;
+ SupportFileSP m_support_file_sp;
/// Keep track of the on-disk checksum.
Checksum m_checksum;
@@ -107,9 +108,9 @@ class SourceManager {
lldb::TargetWP m_target_wp;
private:
- void CommonInitializer(lldb::SupportFileSP support_file_sp,
+ void CommonInitializer(SupportFileSP support_file_sp,
lldb::TargetSP target_sp);
- void CommonInitializerImpl(lldb::SupportFileSP support_file_sp,
+ void CommonInitializerImpl(SupportFileSP support_file_sp,
lldb::TargetSP target_sp);
};
@@ -162,7 +163,7 @@ class SourceManager {
}
size_t DisplaySourceLinesWithLineNumbers(
- lldb::SupportFileSP support_file_sp, uint32_t line, uint32_t column,
+ SupportFileSP support_file_sp, uint32_t line, uint32_t column,
uint32_t context_before, uint32_t context_after,
const char *current_line_cstr, Stream *s,
const SymbolContextList *bp_locs = nullptr);
@@ -176,13 +177,13 @@ class SourceManager {
size_t DisplayMoreWithLineNumbers(Stream *s, uint32_t count, bool reverse,
const SymbolContextList *bp_locs = nullptr);
- bool SetDefaultFileAndLine(lldb::SupportFileSP support_file_sp,
+ bool SetDefaultFileAndLine(SupportFileSP support_file_sp,
uint32_t line);
struct SupportFileAndLine {
- lldb::SupportFileSP support_file_sp;
+ SupportFileSP support_file_sp;
uint32_t line;
- SupportFileAndLine(lldb::SupportFileSP support_file_sp, uint32_t line)
+ SupportFileAndLine(SupportFileSP support_file_sp, uint32_t line)
: support_file_sp(support_file_sp), line(line) {}
};
@@ -192,15 +193,15 @@ class SourceManager {
return (GetFile(m_last_support_file_sp).get() != nullptr);
}
- void FindLinesMatchingRegex(lldb::SupportFileSP support_file_sp,
+ void FindLinesMatchingRegex(SupportFileSP support_file_sp,
RegularExpression ®ex, uint32_t start_line,
uint32_t end_line,
std::vector<uint32_t> &match_lines);
- FileSP GetFile(lldb::SupportFileSP support_file_sp);
+ FileSP GetFile(SupportFileSP support_file_sp);
protected:
- lldb::SupportFileSP m_last_support_file_sp;
+ SupportFileSP m_last_support_file_sp;
uint32_t m_last_line;
uint32_t m_last_count;
bool m_default_set;
diff --git a/lldb/include/lldb/Symbol/CompileUnit.h b/lldb/include/lldb/Symbol/CompileUnit.h
index c5bb080d21184..d537dacdc9b17 100644
--- a/lldb/include/lldb/Symbol/CompileUnit.h
+++ b/lldb/include/lldb/Symbol/CompileUnit.h
@@ -118,7 +118,7 @@ class CompileUnit : public std::enable_shared_from_this<CompileUnit>,
/// An rvalue list of already parsed support files.
/// \see lldb::LanguageType
CompileUnit(const lldb::ModuleSP &module_sp, void *user_data,
- lldb::SupportFileSP support_file_sp, lldb::user_id_t uid,
+ SupportFileSP support_file_sp, lldb::user_id_t uid,
lldb::LanguageType language, lldb_private::LazyBool is_optimized,
SupportFileList &&support_files = {});
@@ -234,7 +234,7 @@ class CompileUnit : public std::enable_shared_from_this<CompileUnit>,
}
/// Return the primary source file associated with this compile unit.
- lldb::SupportFileSP GetPrimarySupportFile() const {
+ SupportFileSP GetPrimarySupportFile() const {
return m_primary_support_file_sp;
}
@@ -430,7 +430,7 @@ class CompileUnit : public std::enable_shared_from_this<CompileUnit>,
/// compile unit.
std::vector<SourceModule> m_imported_modules;
/// The primary file associated with this compile unit.
- lldb::SupportFileSP m_primary_support_file_sp;
+ SupportFileSP m_primary_support_file_sp;
/// Files associated with this compile unit's line table and declarations.
SupportFileList m_support_files;
/// Line table that will get parsed on demand.
diff --git a/lldb/include/lldb/Symbol/Function.h b/lldb/include/lldb/Symbol/Function.h
index 21b3f9ab4a70c..3f390e6edef8f 100644
--- a/lldb/include/lldb/Symbol/Function.h
+++ b/lldb/include/lldb/Symbol/Function.h
@@ -469,12 +469,12 @@ class Function : public UserID, public SymbolContextScope {
///
/// \param[out] line_no
/// The line number.
- void GetStartLineSourceInfo(lldb::SupportFileSP &source_file_sp,
+ void GetStartLineSourceInfo(SupportFileSP &source_file_sp,
uint32_t &line_no);
using SourceRange = Range<uint32_t, uint32_t>;
/// Find the file and line number range of the function.
- llvm::Expected<std::pair<lldb::SupportFileSP, SourceRange>> GetSourceInfo();
+ llvm::Expected<std::pair<SupportFileSP, SourceRange>> GetSourceInfo();
/// Get the outgoing call edges from this function, sorted by their return
/// PC addresses (in increasing order).
diff --git a/lldb/include/lldb/Symbol/LineEntry.h b/lldb/include/lldb/Symbol/LineEntry.h
index 8da59cf0bd24a..c6b414cb9659c 100644
--- a/lldb/include/lldb/Symbol/LineEntry.h
+++ b/lldb/include/lldb/Symbol/LineEntry.h
@@ -137,10 +137,10 @@ struct LineEntry {
AddressRange range;
/// The source file, possibly mapped by the target.source-map setting.
- lldb::SupportFileSP file_sp;
+ SupportFileSP file_sp;
/// The original source file, from debug info.
- lldb::SupportFileSP original_file_sp;
+ SupportFileSP original_file_sp;
/// The source line number, or LLDB_INVALID_LINE_NUMBER if there is no line
/// number information.
diff --git a/lldb/include/lldb/Utility/FileSpecList.h b/lldb/include/lldb/Utility/FileSpecList.h
index d091a9246e082..1ebdf3969f2c6 100644
--- a/lldb/include/lldb/Utility/FileSpecList.h
+++ b/lldb/include/lldb/Utility/FileSpecList.h
@@ -41,7 +41,7 @@ class SupportFileList {
bool AppendIfUnique(const FileSpec &file);
size_t GetSize() const { return m_files.size(); }
const FileSpec &GetFileSpecAtIndex(size_t idx) const;
- lldb::SupportFileSP GetSupportFileAtIndex(size_t idx) const;
+ SupportFileSP GetSupportFileAtIndex(size_t idx) const;
size_t FindFileIndex(size_t idx, const FileSpec &file, bool full) const;
/// Find a compatible file index.
///
diff --git a/lldb/include/lldb/Utility/NonNullSharedPtr.h b/lldb/include/lldb/Utility/NonNullSharedPtr.h
new file mode 100644
index 0000000000000..2f87c34a59bde
--- /dev/null
+++ b/lldb/include/lldb/Utility/NonNullSharedPtr.h
@@ -0,0 +1,96 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_UTILITY_NONNULLSHAREDPTR_H
+#define LLDB_UTILITY_NONNULLSHAREDPTR_H
+
+#include <memory>
+#include <utility>
+
+namespace lldb_private {
+
+/// A non-nullable shared pointer that always holds a valid object.
+///
+/// NonNullSharedPtr is a smart pointer wrapper around std::shared_ptr that
+/// guarantees the pointer is never null. If default-constructed, it creates
+/// a default-constructed instance of T.
+///
+/// This class is used for enforcing invariants at the type level and
+/// eliminating entire classes of null pointer bugs.
+///
+/// @tparam T The type of object to manage. Must be default-constructible.
+template <typename T> class NonNullSharedPtr : private std::shared_ptr<T> {
+ using Base = std::shared_ptr<T>;
+
+public:
+ NonNullSharedPtr() : Base(std::make_shared<T>()) {}
+
+ NonNullSharedPtr(const std::shared_ptr<T> &t)
+ : Base(t ? t : std::make_shared<T>()) {
+ assert(t && "NonNullSharedPtr initialized from NULL shared_ptr");
+ }
+
+ NonNullSharedPtr(std::shared_ptr<T> &&t)
+ : Base(t ? std::move(t) : std::make_shared<T>()) {
+ // Can't assert on t as it's been moved-from.
+ }
+
+ NonNullSharedPtr(const NonNullSharedPtr &other) : Base(other) {}
+
+ NonNullSharedPtr(NonNullSharedPtr &&other) noexcept
+ : Base(std::move(other)) {}
+
+ NonNullSharedPtr &operator=(const NonNullSharedPtr &other) {
+ Base::operator=(other);
+ return *this;
+ }
+
+ NonNullSharedPtr &operator=(NonNullSharedPtr &&other) noexcept {
+ Base::operator=(std::move(other));
+ return *this;
+ }
+
+ using Base::operator*;
+ using Base::operator->;
+ using Base::get;
+ using Base::unique;
+ using Base::use_count;
+ using Base::operator bool;
+
+ void swap(NonNullSharedPtr &other) noexcept { Base::swap(other); }
+
+ /// Explicitly deleted operations that could introduce nullptr.
+ /// @{
+ void reset() = delete;
+ void reset(T *ptr) = delete;
+ /// @}
+};
+
+} // namespace lldb_private
+
+template <typename T>
+bool operator==(const lldb_private::NonNullSharedPtr<T> &lhs,
+ const lldb_private::NonNullSharedPtr<T> &rhs) {
+ return lhs.get() == rhs.get();
+}
+
+template <typename T>
+bool operator!=(const lldb_private::NonNullSharedPtr<T> &lhs,
+ const lldb_private::NonNullSharedPtr<T> &rhs) {
+ return !(lhs == rhs);
+}
+
+/// Specialized swap function for NonNullSharedPtr to enable argument-dependent
+/// lookup (ADL) and efficient swapping.
+template <typename T>
+void swap(lldb_private::NonNullSharedPtr<T> &lhs,
+ lldb_private::NonNullSharedPtr<T> &rhs) noexcept {
+ lhs.swap(rhs);
+}
+
+#endif
diff --git a/lldb/include/lldb/Utility/SupportFile.h b/lldb/include/lldb/Utility/SupportFile.h
index c389edf0e9f12..4bc898906ea35 100644
--- a/lldb/include/lldb/Utility/SupportFile.h
+++ b/lldb/include/lldb/Utility/SupportFile.h
@@ -11,6 +11,7 @@
#include "lldb/Utility/Checksum.h"
#include "lldb/Utility/FileSpec.h"
+#include "lldb/Utility/NonNullSharedPtr.h"
namespace lldb_private {
@@ -76,6 +77,8 @@ class SupportFile {
const Checksum m_checksum;
};
+typedef NonNullSharedPtr<lldb_private::SupportFile> SupportFileSP;
+
} // namespace lldb_private
#endif // LLDB_UTILITY_SUPPORTFILE_H
diff --git a/lldb/include/lldb/lldb-forward.h b/lldb/include/lldb/lldb-forward.h
index 8b8d081ca2113..c8e2e97953aa4 100644
--- a/lldb/include/lldb/lldb-forward.h
+++ b/lldb/include/lldb/lldb-forward.h
@@ -493,7 +493,6 @@ typedef std::shared_ptr<lldb_private::TypeSummaryImpl> TypeSummaryImplSP;
typedef std::shared_ptr<lldb_private::TypeSummaryOptions> TypeSummaryOptionsSP;
typedef std::shared_ptr<lldb_private::ScriptedSyntheticChildren>
ScriptedSyntheticChildrenSP;
-typedef std::shared_ptr<lldb_private::SupportFile> SupportFileSP;
typedef std::shared_ptr<lldb_private::UnixSignals> UnixSignalsSP;
typedef std::weak_ptr<lldb_private::UnixSignals> UnixSignalsWP;
typedef std::shared_ptr<lldb_private::UnwindAssembly> UnwindAssemblySP;
diff --git a/lldb/source/Commands/CommandObjectSource.cpp b/lldb/source/Commands/CommandObjectSource.cpp
index 0b4599b16ef0d..b280e0536b2cf 100644
--- a/lldb/source/Commands/CommandObjectSource.cpp
+++ b/lldb/source/Commands/CommandObjectSource.cpp
@@ -1194,7 +1194,7 @@ class CommandObjectSourceList : public CommandObjectParsed {
// file(s) will be found and assigned to
// sc.comp_unit->GetPrimarySupportFile, which is NOT what we want to
// print. Instead, we want to print the one from the line entry.
- lldb::SupportFileSP found_file_sp = sc.line_entry.file_sp;
+ SupportFileSP found_file_sp = sc.line_entry.file_sp;
target.GetSourceManager().DisplaySourceLinesWithLineNumbers(
found_file_sp, m_options.start_line, column, 0,
diff --git a/lldb/source/Core/SourceManager.cpp b/lldb/source/Core/SourceManager.cpp
index 097173ffe678e..618cc8e411f19 100644
--- a/lldb/source/Core/SourceManager.cpp
+++ b/lldb/source/Core/SourceManager.cpp
@@ -25,6 +25,7 @@
#include "lldb/Target/Target.h"
#include "lldb/Utility/AnsiTerminal.h"
#include "lldb/Utility/ConstString.h"
+#include "lldb/Utility/SupportFile.h"
#include "lldb/Utility/DataBuffer.h"
#include "lldb/Utility/LLDBLog.h"
#include "lldb/Utility/Log.h"
@@ -325,7 +326,7 @@ size_t SourceManager::DisplaySourceLinesWithLineNumbersUsingLastFile(
}
size_t SourceManager::DisplaySourceLinesWithLineNumbers(
- lldb::SupportFileSP support_file_sp, uint32_t line, uint32_t column,
+ SupportFileSP support_file_sp, uint32_t line, uint32_t column,
uint32_t context_before, uint32_t context_after,
const char *current_line_cstr, Stream *s,
const SymbolContextList *bp_locs) {
@@ -389,7 +390,7 @@ size_t SourceManager::DisplayMoreWithLineNumbers(
return 0;
}
-bool SourceManager::SetDefaultFileAndLine(lldb::SupportFileSP support_file_sp,
+bool SourceManager::SetDefaultFileAndLine(SupportFileSP support_file_sp,
uint32_t line) {
assert(support_file_sp && "SupportFile must be valid");
@@ -575,7 +576,7 @@ void SourceManager::File::CommonInitializerImpl(SupportFileSP support_file_sp,
}
}
-void SourceManager::File::SetSupportFile(lldb::SupportFileSP support_file_sp) {
+void SourceManager::File::SetSupportFile(SupportFileSP support_file_sp) {
FileSpec file_spec = support_file_sp->GetSpecOnly();
resolve_tilde(file_spec);
m_support_file_sp =
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index bcb3ad854c388..ff0aa3e6e4257 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -794,7 +794,7 @@ lldb::CompUnitSP SymbolFileDWARF::ParseCompileUnit(DWARFCompileUnit &dwarf_cu) {
} else {
ModuleSP module_sp(m_objfile_sp->GetModule());
if (module_sp) {
- auto initialize_cu = [&](lldb::SupportFileSP support_file_sp,
+ auto initialize_cu = [&](SupportFileSP support_file_sp,
LanguageType cu_language,
SupportFileList &&support_files = {}) {
BuildCuTranslationTable();
diff --git a/lldb/source/Symbol/CompileUnit.cpp b/lldb/source/Symbol/CompileUnit.cpp
index 166f111ef6220..c23b7c4a6a5cb 100644
--- a/lldb/source/Symbol/CompileUnit.cpp
+++ b/lldb/source/Symbol/CompileUnit.cpp
@@ -27,7 +27,7 @@ CompileUnit::CompileUnit(const lldb::ModuleSP &module_sp, void *user_data,
language, is_optimized) {}
CompileUnit::CompileUnit(const lldb::ModuleSP &module_sp, void *user_data,
- lldb::SupportFileSP support_file_sp,
+ SupportFileSP support_file_sp,
const lldb::user_id_t cu_sym_id,
lldb::LanguageType language,
lldb_private::LazyBool is_optimized,
diff --git a/lldb/source/Target/ThreadPlanStepRange.cpp b/lldb/source/Target/ThreadPlanStepRange.cpp
index dca96cc74ba46..20ca1fc3dbb2a 100644
--- a/lldb/source/Target/ThreadPlanStepRange.cpp
+++ b/lldb/source/Target/ThreadPlanStepRange.cpp
@@ -431,7 +431,7 @@ bool ThreadPlanStepRange::SetNextBranchBreakpoint() {
top_most_line_entry.original_file_sp =
std::make_shared<SupportFile>(call_site_file_spec);
top_most_line_entry.range = range;
- top_most_line_entry.file_sp.reset();
+ top_most_line_entry.file_sp = std::make_shared<SupportFile>();
top_most_line_entry.ApplyFileMappings(
GetThread().CalculateTarget());
if (!top_most_line_entry.file_sp)
diff --git a/lldb/source/Utility/FileSpecList.cpp b/lldb/source/Utility/FileSpecList.cpp
index 5852367f77827..a4076ee815719 100644
--- a/lldb/source/Utility/FileSpecList.cpp
+++ b/lldb/source/Utility/FileSpecList.cpp
@@ -46,7 +46,7 @@ bool FileSpecList::AppendIfUnique(const FileSpec &file_spec) {
bool SupportFileList::AppendIfUnique(const FileSpec &file_spec) {
collection::iterator end = m_files.end();
if (find_if(m_files.begin(), end,
- [&](const std::shared_ptr<SupportFile> &support_file) {
+ [&](const SupportFileSP &support_file) {
return support_file->GetSpecOnly() == file_spec;
}) == end) {
Append(file_spec);
@@ -214,7 +214,7 @@ const FileSpec &SupportFileList::GetFileSpecAtIndex(size_t idx) const {
return g_empty_file_spec;
}
-std::shared_ptr<SupportFile>
+SupportFileSP
SupportFileList::GetSupportFileAtIndex(size_t idx) const {
if (idx < m_files.size())
return m_files[idx];
|
🐧 Linux x64 Test Results
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| using Base = std::shared_ptr<T>; | ||
|
|
||
| public: | ||
| NonNullSharedPtr() : Base(std::make_shared<T>()) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this assert that make_shared succeeded in creating a non-null shared_ptr? It's rare but we have exceptions disabled, so this will violate the invariant under your nose and you'll crash somewhere later on anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind adding it, but at the same time, if malloc failed, we have bigger issues, and nothing in LLDB is resilient against that... so it would just be theater.
|
|
||
| NonNullSharedPtr(std::shared_ptr<T> &&t) | ||
| : Base(t ? std::move(t) : std::make_shared<T>()) { | ||
| // Can't assert on t as it's been moved-from. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't assert on t, but you can assert that you're non-null, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but that could only happen if make_shared failed, and then we're in the scenario described in the previous comment.
This patch fixes and eliminates the possibility of SupportFileSP ever being nullptr. The support file was originally treated like a value type, but became a polymorphic type and therefore has to be stored and passed around as a pointer.
To avoid having all the callers check the validity of the pointer, I introduced the invariant that SupportFileSP is never null and always default constructed. However, without enforcement at the type level, that's fragile and indeed, we already identified two crashes where someone accidentally broke that invariant.
This PR introduces a NonNullSharedPtr to prevent that. NonNullSharedPtr is a smart pointer wrapper around std::shared_ptr that guarantees the pointer is never null. If default-constructed, it creates a default-constructed instance of the contained type.
rdar://164989579