Skip to content

Commit daea9d9

Browse files
committed
[lldb] Eliminate SupportFileSP nullptr derefs
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
1 parent 3f61402 commit daea9d9

File tree

14 files changed

+132
-32
lines changed

14 files changed

+132
-32
lines changed

lldb/include/lldb/Core/SourceManager.h

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
#include "lldb/Utility/Checksum.h"
1313
#include "lldb/Utility/FileSpec.h"
14+
#include "lldb/Utility/SupportFile.h"
1415
#include "lldb/lldb-defines.h"
1516
#include "lldb/lldb-forward.h"
1617

@@ -38,8 +39,8 @@ class SourceManager {
3839
const SourceManager::File &rhs);
3940

4041
public:
41-
File(lldb::SupportFileSP support_file_sp, lldb::TargetSP target_sp);
42-
File(lldb::SupportFileSP support_file_sp, lldb::DebuggerSP debugger_sp);
42+
File(SupportFileSP support_file_sp, lldb::TargetSP target_sp);
43+
File(SupportFileSP support_file_sp, lldb::DebuggerSP debugger_sp);
4344

4445
bool ModificationTimeIsStale() const;
4546
bool PathRemappingIsStale() const;
@@ -57,7 +58,7 @@ class SourceManager {
5758

5859
bool LineIsValid(uint32_t line);
5960

60-
lldb::SupportFileSP GetSupportFile() const {
61+
SupportFileSP GetSupportFile() const {
6162
assert(m_support_file_sp && "SupportFileSP must always be valid");
6263
return m_support_file_sp;
6364
}
@@ -80,13 +81,13 @@ class SourceManager {
8081

8182
protected:
8283
/// Set file and update modification time.
83-
void SetSupportFile(lldb::SupportFileSP support_file_sp);
84+
void SetSupportFile(SupportFileSP support_file_sp);
8485

8586
bool CalculateLineOffsets(uint32_t line = UINT32_MAX);
8687

8788
/// The support file. If the target has source mappings, this might be
8889
/// different from the original support file passed to the constructor.
89-
lldb::SupportFileSP m_support_file_sp;
90+
SupportFileSP m_support_file_sp;
9091

9192
/// Keep track of the on-disk checksum.
9293
Checksum m_checksum;
@@ -107,9 +108,9 @@ class SourceManager {
107108
lldb::TargetWP m_target_wp;
108109

109110
private:
110-
void CommonInitializer(lldb::SupportFileSP support_file_sp,
111+
void CommonInitializer(SupportFileSP support_file_sp,
111112
lldb::TargetSP target_sp);
112-
void CommonInitializerImpl(lldb::SupportFileSP support_file_sp,
113+
void CommonInitializerImpl(SupportFileSP support_file_sp,
113114
lldb::TargetSP target_sp);
114115
};
115116

@@ -162,7 +163,7 @@ class SourceManager {
162163
}
163164

164165
size_t DisplaySourceLinesWithLineNumbers(
165-
lldb::SupportFileSP support_file_sp, uint32_t line, uint32_t column,
166+
SupportFileSP support_file_sp, uint32_t line, uint32_t column,
166167
uint32_t context_before, uint32_t context_after,
167168
const char *current_line_cstr, Stream *s,
168169
const SymbolContextList *bp_locs = nullptr);
@@ -176,13 +177,13 @@ class SourceManager {
176177
size_t DisplayMoreWithLineNumbers(Stream *s, uint32_t count, bool reverse,
177178
const SymbolContextList *bp_locs = nullptr);
178179

179-
bool SetDefaultFileAndLine(lldb::SupportFileSP support_file_sp,
180+
bool SetDefaultFileAndLine(SupportFileSP support_file_sp,
180181
uint32_t line);
181182

182183
struct SupportFileAndLine {
183-
lldb::SupportFileSP support_file_sp;
184+
SupportFileSP support_file_sp;
184185
uint32_t line;
185-
SupportFileAndLine(lldb::SupportFileSP support_file_sp, uint32_t line)
186+
SupportFileAndLine(SupportFileSP support_file_sp, uint32_t line)
186187
: support_file_sp(support_file_sp), line(line) {}
187188
};
188189

@@ -192,15 +193,15 @@ class SourceManager {
192193
return (GetFile(m_last_support_file_sp).get() != nullptr);
193194
}
194195

195-
void FindLinesMatchingRegex(lldb::SupportFileSP support_file_sp,
196+
void FindLinesMatchingRegex(SupportFileSP support_file_sp,
196197
RegularExpression &regex, uint32_t start_line,
197198
uint32_t end_line,
198199
std::vector<uint32_t> &match_lines);
199200

200-
FileSP GetFile(lldb::SupportFileSP support_file_sp);
201+
FileSP GetFile(SupportFileSP support_file_sp);
201202

202203
protected:
203-
lldb::SupportFileSP m_last_support_file_sp;
204+
SupportFileSP m_last_support_file_sp;
204205
uint32_t m_last_line;
205206
uint32_t m_last_count;
206207
bool m_default_set;

lldb/include/lldb/Symbol/CompileUnit.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ class CompileUnit : public std::enable_shared_from_this<CompileUnit>,
118118
/// An rvalue list of already parsed support files.
119119
/// \see lldb::LanguageType
120120
CompileUnit(const lldb::ModuleSP &module_sp, void *user_data,
121-
lldb::SupportFileSP support_file_sp, lldb::user_id_t uid,
121+
SupportFileSP support_file_sp, lldb::user_id_t uid,
122122
lldb::LanguageType language, lldb_private::LazyBool is_optimized,
123123
SupportFileList &&support_files = {});
124124

@@ -234,7 +234,7 @@ class CompileUnit : public std::enable_shared_from_this<CompileUnit>,
234234
}
235235

236236
/// Return the primary source file associated with this compile unit.
237-
lldb::SupportFileSP GetPrimarySupportFile() const {
237+
SupportFileSP GetPrimarySupportFile() const {
238238
return m_primary_support_file_sp;
239239
}
240240

@@ -430,7 +430,7 @@ class CompileUnit : public std::enable_shared_from_this<CompileUnit>,
430430
/// compile unit.
431431
std::vector<SourceModule> m_imported_modules;
432432
/// The primary file associated with this compile unit.
433-
lldb::SupportFileSP m_primary_support_file_sp;
433+
SupportFileSP m_primary_support_file_sp;
434434
/// Files associated with this compile unit's line table and declarations.
435435
SupportFileList m_support_files;
436436
/// Line table that will get parsed on demand.

lldb/include/lldb/Symbol/Function.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -469,12 +469,12 @@ class Function : public UserID, public SymbolContextScope {
469469
///
470470
/// \param[out] line_no
471471
/// The line number.
472-
void GetStartLineSourceInfo(lldb::SupportFileSP &source_file_sp,
472+
void GetStartLineSourceInfo(SupportFileSP &source_file_sp,
473473
uint32_t &line_no);
474474

475475
using SourceRange = Range<uint32_t, uint32_t>;
476476
/// Find the file and line number range of the function.
477-
llvm::Expected<std::pair<lldb::SupportFileSP, SourceRange>> GetSourceInfo();
477+
llvm::Expected<std::pair<SupportFileSP, SourceRange>> GetSourceInfo();
478478

479479
/// Get the outgoing call edges from this function, sorted by their return
480480
/// PC addresses (in increasing order).

lldb/include/lldb/Symbol/LineEntry.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -137,10 +137,10 @@ struct LineEntry {
137137
AddressRange range;
138138

139139
/// The source file, possibly mapped by the target.source-map setting.
140-
lldb::SupportFileSP file_sp;
140+
SupportFileSP file_sp;
141141

142142
/// The original source file, from debug info.
143-
lldb::SupportFileSP original_file_sp;
143+
SupportFileSP original_file_sp;
144144

145145
/// The source line number, or LLDB_INVALID_LINE_NUMBER if there is no line
146146
/// number information.

lldb/include/lldb/Utility/FileSpecList.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ class SupportFileList {
4141
bool AppendIfUnique(const FileSpec &file);
4242
size_t GetSize() const { return m_files.size(); }
4343
const FileSpec &GetFileSpecAtIndex(size_t idx) const;
44-
lldb::SupportFileSP GetSupportFileAtIndex(size_t idx) const;
44+
SupportFileSP GetSupportFileAtIndex(size_t idx) const;
4545
size_t FindFileIndex(size_t idx, const FileSpec &file, bool full) const;
4646
/// Find a compatible file index.
4747
///
Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
//===----------------------------------------------------------------------===//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
9+
#ifndef LLDB_UTILITY_NONNULLSHAREDPTR_H
10+
#define LLDB_UTILITY_NONNULLSHAREDPTR_H
11+
12+
#include <memory>
13+
#include <utility>
14+
15+
namespace lldb_private {
16+
17+
/// A non-nullable shared pointer that always holds a valid object.
18+
///
19+
/// NonNullSharedPtr is a smart pointer wrapper around std::shared_ptr that
20+
/// guarantees the pointer is never null. If default-constructed, it creates
21+
/// a default-constructed instance of T.
22+
///
23+
/// This class is used for enforcing invariants at the type level and
24+
/// eliminating entire classes of null pointer bugs.
25+
///
26+
/// @tparam T The type of object to manage. Must be default-constructible.
27+
template <typename T> class NonNullSharedPtr : private std::shared_ptr<T> {
28+
using Base = std::shared_ptr<T>;
29+
30+
public:
31+
NonNullSharedPtr() : Base(std::make_shared<T>()) {}
32+
33+
NonNullSharedPtr(const std::shared_ptr<T> &t)
34+
: Base(t ? t : std::make_shared<T>()) {
35+
assert(t && "NonNullSharedPtr initialized from NULL shared_ptr");
36+
}
37+
38+
NonNullSharedPtr(std::shared_ptr<T> &&t)
39+
: Base(t ? std::move(t) : std::make_shared<T>()) {
40+
// Can't assert on t as it's been moved-from.
41+
}
42+
43+
NonNullSharedPtr(const NonNullSharedPtr &other) : Base(other) {}
44+
45+
NonNullSharedPtr(NonNullSharedPtr &&other) noexcept
46+
: Base(std::move(other)) {}
47+
48+
NonNullSharedPtr &operator=(const NonNullSharedPtr &other) {
49+
Base::operator=(other);
50+
return *this;
51+
}
52+
53+
NonNullSharedPtr &operator=(NonNullSharedPtr &&other) noexcept {
54+
Base::operator=(std::move(other));
55+
return *this;
56+
}
57+
58+
using Base::operator*;
59+
using Base::operator->;
60+
using Base::get;
61+
using Base::unique;
62+
using Base::use_count;
63+
using Base::operator bool;
64+
65+
void swap(NonNullSharedPtr &other) noexcept { Base::swap(other); }
66+
67+
/// Explicitly deleted operations that could introduce nullptr.
68+
/// @{
69+
void reset() = delete;
70+
void reset(T *ptr) = delete;
71+
/// @}
72+
};
73+
74+
} // namespace lldb_private
75+
76+
template <typename T>
77+
bool operator==(const lldb_private::NonNullSharedPtr<T> &lhs,
78+
const lldb_private::NonNullSharedPtr<T> &rhs) {
79+
return lhs.get() == rhs.get();
80+
}
81+
82+
template <typename T>
83+
bool operator!=(const lldb_private::NonNullSharedPtr<T> &lhs,
84+
const lldb_private::NonNullSharedPtr<T> &rhs) {
85+
return !(lhs == rhs);
86+
}
87+
88+
/// Specialized swap function for NonNullSharedPtr to enable argument-dependent
89+
/// lookup (ADL) and efficient swapping.
90+
template <typename T>
91+
void swap(lldb_private::NonNullSharedPtr<T> &lhs,
92+
lldb_private::NonNullSharedPtr<T> &rhs) noexcept {
93+
lhs.swap(rhs);
94+
}
95+
96+
#endif

lldb/include/lldb/Utility/SupportFile.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
#include "lldb/Utility/Checksum.h"
1313
#include "lldb/Utility/FileSpec.h"
14+
#include "lldb/Utility/NonNullSharedPtr.h"
1415

1516
namespace lldb_private {
1617

@@ -76,6 +77,8 @@ class SupportFile {
7677
const Checksum m_checksum;
7778
};
7879

80+
typedef NonNullSharedPtr<lldb_private::SupportFile> SupportFileSP;
81+
7982
} // namespace lldb_private
8083

8184
#endif // LLDB_UTILITY_SUPPORTFILE_H

lldb/include/lldb/lldb-forward.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -493,7 +493,6 @@ typedef std::shared_ptr<lldb_private::TypeSummaryImpl> TypeSummaryImplSP;
493493
typedef std::shared_ptr<lldb_private::TypeSummaryOptions> TypeSummaryOptionsSP;
494494
typedef std::shared_ptr<lldb_private::ScriptedSyntheticChildren>
495495
ScriptedSyntheticChildrenSP;
496-
typedef std::shared_ptr<lldb_private::SupportFile> SupportFileSP;
497496
typedef std::shared_ptr<lldb_private::UnixSignals> UnixSignalsSP;
498497
typedef std::weak_ptr<lldb_private::UnixSignals> UnixSignalsWP;
499498
typedef std::shared_ptr<lldb_private::UnwindAssembly> UnwindAssemblySP;

lldb/source/Commands/CommandObjectSource.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1194,7 +1194,7 @@ class CommandObjectSourceList : public CommandObjectParsed {
11941194
// file(s) will be found and assigned to
11951195
// sc.comp_unit->GetPrimarySupportFile, which is NOT what we want to
11961196
// print. Instead, we want to print the one from the line entry.
1197-
lldb::SupportFileSP found_file_sp = sc.line_entry.file_sp;
1197+
SupportFileSP found_file_sp = sc.line_entry.file_sp;
11981198

11991199
target.GetSourceManager().DisplaySourceLinesWithLineNumbers(
12001200
found_file_sp, m_options.start_line, column, 0,

lldb/source/Core/SourceManager.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
#include "lldb/Target/Target.h"
2626
#include "lldb/Utility/AnsiTerminal.h"
2727
#include "lldb/Utility/ConstString.h"
28+
#include "lldb/Utility/SupportFile.h"
2829
#include "lldb/Utility/DataBuffer.h"
2930
#include "lldb/Utility/LLDBLog.h"
3031
#include "lldb/Utility/Log.h"
@@ -325,7 +326,7 @@ size_t SourceManager::DisplaySourceLinesWithLineNumbersUsingLastFile(
325326
}
326327

327328
size_t SourceManager::DisplaySourceLinesWithLineNumbers(
328-
lldb::SupportFileSP support_file_sp, uint32_t line, uint32_t column,
329+
SupportFileSP support_file_sp, uint32_t line, uint32_t column,
329330
uint32_t context_before, uint32_t context_after,
330331
const char *current_line_cstr, Stream *s,
331332
const SymbolContextList *bp_locs) {
@@ -389,7 +390,7 @@ size_t SourceManager::DisplayMoreWithLineNumbers(
389390
return 0;
390391
}
391392

392-
bool SourceManager::SetDefaultFileAndLine(lldb::SupportFileSP support_file_sp,
393+
bool SourceManager::SetDefaultFileAndLine(SupportFileSP support_file_sp,
393394
uint32_t line) {
394395
assert(support_file_sp && "SupportFile must be valid");
395396

@@ -575,7 +576,7 @@ void SourceManager::File::CommonInitializerImpl(SupportFileSP support_file_sp,
575576
}
576577
}
577578

578-
void SourceManager::File::SetSupportFile(lldb::SupportFileSP support_file_sp) {
579+
void SourceManager::File::SetSupportFile(SupportFileSP support_file_sp) {
579580
FileSpec file_spec = support_file_sp->GetSpecOnly();
580581
resolve_tilde(file_spec);
581582
m_support_file_sp =

0 commit comments

Comments
 (0)