Skip to content

Commit 486c329

Browse files
committed
Warn when unique objects might be duplicated in shared libraries
When a hidden object is built into multiple shared libraries, each instance of the library will get its own copy. If the object was supposed to be globally unique (e.g. a global variable or static member), this can cause very subtle bugs. An object might be incorrectly duplicated if it: - Is defined in a header (so it might appear in multiple TUs), and - Has external linkage (otherwise it's supposed to be duplicated), and - Has hidden visibility (or else the dynamic linker will handle it) The duplication is only a problem if one of the following is true: 1. The object is mutable (the copies won't be in sync), or 2. Its initialization has side effects (it may now run more than once), or 3. The value of its address is used (which one?). To detect this, we add a new -Wunique-object-duplication warning. It warns on cases (1) and (2) above. To be conservative, we only warn in case (2) if we are certain the initializer has side effects, and we don't warn on `new` because the only side effect is some extra memory usage. We don't currently warn on case (3) because doing so is prone to false positives: there are many reasons for taking the address which aren't inherently problematic (e.g. passing to a function that expects a pointer). We only run into problems if the code inspects the value of the address. The check is currently disabled for windows, which uses its own analogue of visibility (declimport/declexport). The check is also disabled inside templates, since it can give false positives if a template is never instantiated. Because the warning fires in several places, included third-party libraries, it is currently disabled-by-default until all these instances have been fixed. It should be enabled-by-default afterwards.
1 parent c3b7894 commit 486c329

File tree

6 files changed

+320
-0
lines changed

6 files changed

+320
-0
lines changed

clang/include/clang/Basic/DiagnosticGroups.td

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -694,6 +694,7 @@ def SuspiciousMemaccess : DiagGroup<"suspicious-memaccess",
694694
NonTrivialMemaccess, MemsetTransposedArgs, SuspiciousBzero]>;
695695
def StaticInInline : DiagGroup<"static-in-inline">;
696696
def StaticLocalInInline : DiagGroup<"static-local-in-inline">;
697+
def UniqueObjectDuplication : DiagGroup<"unique-object-duplication">;
697698
def GNUStaticFloatInit : DiagGroup<"gnu-static-float-init">;
698699
def StaticFloatInit : DiagGroup<"static-float-init", [GNUStaticFloatInit]>;
699700
// Allow differentiation between GNU statement expressions in a macro versus

clang/include/clang/Basic/DiagnosticSemaKinds.td

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6169,6 +6169,15 @@ def warn_static_local_in_extern_inline : Warning<
61696169
def note_convert_inline_to_static : Note<
61706170
"use 'static' to give inline function %0 internal linkage">;
61716171

6172+
def warn_possible_object_duplication_mutable : Warning<
6173+
"%0 is mutable, has hidden visibility, and external linkage; it may be "
6174+
"duplicated when built into a shared library">,
6175+
InGroup<UniqueObjectDuplication>, DefaultIgnore;
6176+
def warn_possible_object_duplication_init : Warning<
6177+
"%0 has hidden visibility, and external linkage; its initialization may run "
6178+
"more than once when built into a shared library">,
6179+
InGroup<UniqueObjectDuplication>, DefaultIgnore;
6180+
61726181
def ext_redefinition_of_typedef : ExtWarn<
61736182
"redefinition of typedef %0 is a C11 feature">,
61746183
InGroup<DiagGroup<"typedef-redefinition"> >;

clang/include/clang/Sema/Sema.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3664,6 +3664,12 @@ class Sema final : public SemaBase {
36643664
NonTrivialCUnionContext UseContext,
36653665
unsigned NonTrivialKind);
36663666

3667+
/// Certain globally-unique variables might be accidentally duplicated if
3668+
/// built into multiple shared libraries with hidden visibility. This can
3669+
/// cause problems if the variable is mutable, its initialization is
3670+
/// effectful, or its address is taken.
3671+
bool GloballyUniqueObjectMightBeAccidentallyDuplicated(const VarDecl *dcl);
3672+
36673673
/// AddInitializerToDecl - Adds the initializer Init to the
36683674
/// declaration dcl. If DirectInit is true, this is C++ direct
36693675
/// initialization rather than copy initialization.

clang/lib/Sema/SemaDecl.cpp

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13380,6 +13380,62 @@ void Sema::checkNonTrivialCUnion(QualType QT, SourceLocation Loc,
1338013380
.visit(QT, nullptr, false);
1338113381
}
1338213382

13383+
bool Sema::GloballyUniqueObjectMightBeAccidentallyDuplicated(
13384+
const VarDecl *dcl) {
13385+
if (!dcl || !getLangOpts().CPlusPlus)
13386+
return false;
13387+
13388+
// If an object is defined in a source file, its definition can't get
13389+
// duplicated since it will never appear in more than one TU.
13390+
if (dcl->getASTContext().getSourceManager().isInMainFile(dcl->getLocation()))
13391+
return false;
13392+
13393+
// We only need to warn if the definition is in a header file, so wait to
13394+
// diagnose until we've seen the definition.
13395+
if (!dcl->isThisDeclarationADefinition())
13396+
return false;
13397+
13398+
// If the variable we're looking at is a static local, then we actually care
13399+
// about the properties of the function containing it.
13400+
const ValueDecl *target = dcl;
13401+
// VarDecls and FunctionDecls have different functions for checking
13402+
// inline-ness, so we have to do it manually.
13403+
bool target_is_inline = dcl->isInline();
13404+
13405+
// Update the target and target_is_inline property if necessary
13406+
if (dcl->isStaticLocal()) {
13407+
const DeclContext *ctx = dcl->getDeclContext();
13408+
if (!ctx)
13409+
return false;
13410+
13411+
const FunctionDecl *f_dcl =
13412+
dyn_cast_if_present<FunctionDecl>(ctx->getNonClosureAncestor());
13413+
if (!f_dcl)
13414+
return false;
13415+
13416+
target = f_dcl;
13417+
// IsInlined() checks for the C++ inline property
13418+
target_is_inline = f_dcl->isInlined();
13419+
}
13420+
13421+
// Non-inline variables can only legally appear in one TU
13422+
// FIXME: This also applies to templated variables, but that can rarely lead
13423+
// to false positives so templates are disabled for now.
13424+
if (!target_is_inline)
13425+
return false;
13426+
13427+
// If the object isn't hidden, the dynamic linker will prevent duplication.
13428+
clang::LinkageInfo lnk = target->getLinkageAndVisibility();
13429+
if (lnk.getVisibility() != HiddenVisibility)
13430+
return false;
13431+
13432+
// If the obj doesn't have external linkage, it's supposed to be duplicated.
13433+
if (!isExternalFormalLinkage(lnk.getLinkage()))
13434+
return false;
13435+
13436+
return true;
13437+
}
13438+
1338313439
void Sema::AddInitializerToDecl(Decl *RealDecl, Expr *Init, bool DirectInit) {
1338413440
// If there is no declaration, there was an error parsing it. Just ignore
1338513441
// the initializer.
@@ -14786,6 +14842,51 @@ void Sema::FinalizeDeclaration(Decl *ThisDecl) {
1478614842
if (DC->getRedeclContext()->isFileContext() && VD->isExternallyVisible())
1478714843
AddPushedVisibilityAttribute(VD);
1478814844

14845+
// If this object has external linkage and hidden visibility, it might be
14846+
// duplicated when built into a shared library, which causes problems if it's
14847+
// mutable (since the copies won't be in sync) or its initialization has side
14848+
// effects (since it will run once per copy instead of once globally)
14849+
// FIXME: Windows uses dllexport/dllimport instead of visibility, and we don't
14850+
// handle that yet. Disable the warning on Windows for now.
14851+
// FIXME: Checking templates can cause false positives if the template in
14852+
// question is never instantiated (e.g. only specialized templates are used).
14853+
if (!Context.getTargetInfo().shouldDLLImportComdatSymbols() &&
14854+
!VD->isTemplated() &&
14855+
GloballyUniqueObjectMightBeAccidentallyDuplicated(VD)) {
14856+
// Check mutability. For pointers, ensure that both the pointer and the
14857+
// pointee are (recursively) const.
14858+
QualType Type = VD->getType().getNonReferenceType();
14859+
if (!Type.isConstant(VD->getASTContext())) {
14860+
Diag(VD->getLocation(), diag::warn_possible_object_duplication_mutable)
14861+
<< VD;
14862+
} else {
14863+
while (Type->isPointerType()) {
14864+
Type = Type->getPointeeType();
14865+
if (Type->isFunctionType())
14866+
break;
14867+
if (!Type.isConstant(VD->getASTContext())) {
14868+
Diag(VD->getLocation(),
14869+
diag::warn_possible_object_duplication_mutable)
14870+
<< VD;
14871+
break;
14872+
}
14873+
}
14874+
}
14875+
14876+
// To keep false positives low, only warn if we're certain that the
14877+
// initializer has side effects. Don't warn on operator new, since a mutable
14878+
// pointer will trigger the previous warning, and an immutable pointer
14879+
// getting duplicated just results in a little extra memory usage.
14880+
const Expr *Init = VD->getAnyInitializer();
14881+
if (Init &&
14882+
Init->HasSideEffects(VD->getASTContext(),
14883+
/*IncludePossibleEffects=*/false) &&
14884+
!isa<CXXNewExpr>(Init->IgnoreParenImpCasts())) {
14885+
Diag(Init->getExprLoc(), diag::warn_possible_object_duplication_init)
14886+
<< VD;
14887+
}
14888+
}
14889+
1478914890
// FIXME: Warn on unused var template partial specializations.
1479014891
if (VD->isFileVarDecl() && !isa<VarTemplatePartialSpecializationDecl>(VD))
1479114892
MarkUnusedFileScopedDecl(VD);
Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
// RUN: %clang_cc1 -fsyntax-only -verify=hidden -Wunique-object-duplication -fvisibility=hidden -Wno-unused-value %s
2+
// RUN: %clang_cc1 -fsyntax-only -verify -Wunique-object-duplication -Wno-unused-value %s
3+
// The check is currently disabled on windows. The test should fail because we're not getting the expected warnings.
4+
// XFAIL: target={{.*}}-windows{{.*}}
5+
6+
#include "unique_object_duplication.h"
7+
8+
// Everything in these namespaces here is defined in the cpp file,
9+
// so won't get duplicated
10+
11+
namespace GlobalTest {
12+
float Test::allowedStaticMember1 = 2.3;
13+
}
14+
15+
bool disallowed4 = true;
16+
constexpr inline bool disallowed5 = true;
Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,187 @@
1+
/**
2+
* When building shared libraries, hidden objects which are defined in header
3+
* files will be duplicated, with one copy in each shared library. If the object
4+
* was meant to be globally unique (one copy per program), this can cause very
5+
* subtle bugs. This file contains tests for the -Wunique-object-duplication
6+
* warning, which is meant to detect this.
7+
*
8+
* Roughly, an object might be incorrectly duplicated if:
9+
* - Is defined in a header (so it might appear in multiple TUs), and
10+
* - Has external linkage (otherwise it's supposed to be duplicated), and
11+
* - Has hidden visibility (or else the dynamic linker will handle it)
12+
*
13+
* Duplication becomes an issue only if one of the following is true:
14+
* - The object is mutable (the copies won't be in sync), or
15+
* - Its initialization may has side effects (it may now run more than once), or
16+
* - The value of its address is used.
17+
*
18+
* Currently, we only detect the first two, and only warn on effectful
19+
* initialization if we're certain there are side effects. Warning if the
20+
* address is taken is prone to false positives, so we don't warn for now.
21+
*
22+
* The check is also disabled on Windows for now, since it uses
23+
* dllimport/dllexport instead of visibility.
24+
*/
25+
26+
#define HIDDEN __attribute__((visibility("hidden")))
27+
#define DEFAULT __attribute__((visibility("default")))
28+
29+
// Helper functions
30+
constexpr int init_constexpr(int x) { return x; };
31+
extern double init_dynamic(int);
32+
33+
/******************************************************************************
34+
* Case one: Static local variables in an externally-visible function
35+
******************************************************************************/
36+
namespace StaticLocalTest {
37+
38+
inline void has_static_locals_external() {
39+
// Mutable
40+
static int disallowedStatic1 = 0; // hidden-warning {{'disallowedStatic1' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
41+
// Initialization might run more than once
42+
static const double disallowedStatic2 = disallowedStatic1++; // hidden-warning {{'disallowedStatic2' has hidden visibility, and external linkage; its initialization may run more than once when built into a shared library}}
43+
44+
// OK, because immutable and compile-time-initialized
45+
static constexpr int allowedStatic1 = 0;
46+
static const float allowedStatic2 = 1;
47+
static constexpr int allowedStatic3 = init_constexpr(2);
48+
static const int allowedStatic4 = init_constexpr(3);
49+
}
50+
51+
// Don't warn for non-inline functions, since they can't (legally) appear
52+
// in more than one TU in the first place.
53+
void has_static_locals_non_inline() {
54+
// Mutable
55+
static int allowedStatic1 = 0;
56+
// Initialization might run more than once
57+
static const double allowedStatic2 = allowedStatic1++;
58+
}
59+
60+
// Everything in this function is OK because the function is TU-local
61+
static void has_static_locals_internal() {
62+
static int allowedStatic1 = 0;
63+
static double allowedStatic2 = init_dynamic(2);
64+
static char allowedStatic3 = []() { return allowedStatic1++; }();
65+
66+
static constexpr int allowedStatic4 = 0;
67+
static const float allowedStatic5 = 1;
68+
static constexpr int allowedStatic6 = init_constexpr(2);
69+
static const int allowedStatic7 = init_constexpr(3);
70+
}
71+
72+
namespace {
73+
74+
// Everything in this function is OK because the function is also TU-local
75+
void has_static_locals_anon() {
76+
static int allowedStatic1 = 0;
77+
static double allowedStatic2 = init_dynamic(2);
78+
static char allowedStatic3 = []() { return allowedStatic1++; }();
79+
80+
static constexpr int allowedStatic4 = 0;
81+
static const float allowedStatic5 = 1;
82+
static constexpr int allowedStatic6 = init_constexpr(2);
83+
static const int allowedStatic7 = init_constexpr(3);
84+
}
85+
86+
} // Anonymous namespace
87+
88+
HIDDEN inline void static_local_always_hidden() {
89+
static int disallowedStatic1 = 3; // hidden-warning {{'disallowedStatic1' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
90+
// expected-warning@-1 {{'disallowedStatic1' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
91+
{
92+
static int disallowedStatic2 = 3; // hidden-warning {{'disallowedStatic2' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
93+
// expected-warning@-1 {{'disallowedStatic2' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
94+
}
95+
96+
auto lmb = []() {
97+
static int disallowedStatic3 = 3; // hidden-warning {{'disallowedStatic3' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
98+
// expected-warning@-1 {{'disallowedStatic3' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
99+
};
100+
}
101+
102+
DEFAULT void static_local_never_hidden() {
103+
static int allowedStatic1 = 3;
104+
105+
{
106+
static int allowedStatic2 = 3;
107+
}
108+
109+
auto lmb = []() {
110+
static int allowedStatic3 = 3;
111+
};
112+
}
113+
114+
// Don't warn on this because it's not in a function
115+
const int setByLambda = ([]() { static int x = 3; return x++; })();
116+
117+
inline void has_extern_local() {
118+
extern int allowedAddressExtern; // Not a definition
119+
}
120+
121+
inline void has_regular_local() {
122+
int allowedAddressLocal = 0;
123+
}
124+
125+
inline void has_thread_local() {
126+
// thread_local variables are static by default
127+
thread_local int disallowedThreadLocal = 0; // hidden-warning {{'disallowedThreadLocal' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
128+
}
129+
130+
} // namespace StaticLocalTest
131+
132+
/******************************************************************************
133+
* Case two: Globals with external linkage
134+
******************************************************************************/
135+
namespace GlobalTest {
136+
// Mutable
137+
inline float disallowedGlobal1 = 3.14; // hidden-warning {{'disallowedGlobal1' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
138+
// Same as above, but explicitly marked inline
139+
inline float disallowedGlobal4 = 3.14; // hidden-warning {{'disallowedGlobal4' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
140+
141+
// Initialization might run more than once
142+
inline const double disallowedGlobal5 = disallowedGlobal1++; // hidden-warning {{'disallowedGlobal5' has hidden visibility, and external linkage; its initialization may run more than once when built into a shared library}}
143+
144+
// OK because internal linkage, so duplication is intended
145+
static float allowedGlobal1 = 3.14;
146+
const double allowedGlobal2 = init_dynamic(2);
147+
static const char allowedGlobal3 = []() { return disallowedGlobal1++; }();
148+
static inline double allowedGlobal4 = init_dynamic(2);
149+
150+
// OK, because immutable and compile-time-initialized
151+
constexpr int allowedGlobal5 = 0;
152+
const float allowedGlobal6 = 1;
153+
constexpr int allowedGlobal7 = init_constexpr(2);
154+
const int allowedGlobal8 = init_constexpr(3);
155+
156+
// We don't warn on this because non-inline variables can't (legally) appear
157+
// in more than one TU.
158+
float allowedGlobal9 = 3.14;
159+
160+
// Pointers need to be double-const-qualified
161+
inline float& nonConstReference = disallowedGlobal1; // hidden-warning {{'nonConstReference' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
162+
const inline int& constReference = allowedGlobal5;
163+
164+
inline int* nonConstPointerToNonConst = nullptr; // hidden-warning {{'nonConstPointerToNonConst' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
165+
inline int const* nonConstPointerToConst = nullptr; // hidden-warning {{'nonConstPointerToConst' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
166+
inline int* const constPointerToNonConst = nullptr; // hidden-warning {{'constPointerToNonConst' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
167+
inline int const* const constPointerToConst = nullptr;
168+
// Don't warn on new because it tends to generate false positives
169+
inline int const* const constPointerToConstNew = new int(7);
170+
171+
inline int const * const * const * const nestedConstPointer = nullptr;
172+
inline int const * const ** const * const nestedNonConstPointer = nullptr; // hidden-warning {{'nestedNonConstPointer' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
173+
174+
struct Test {
175+
static inline float disallowedStaticMember1; // hidden-warning {{'disallowedStaticMember1' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
176+
// Defined below, in the header file
177+
static float disallowedStaticMember2;
178+
// Defined in the cpp file, so won't get duplicated
179+
static float allowedStaticMember1;
180+
181+
// Tests here are sparse because the AddrTest case below will define plenty
182+
// more, which aren't problematic to define (because they're immutable), but
183+
// may still cause problems if their address is taken.
184+
};
185+
186+
inline float Test::disallowedStaticMember2 = 2.3; // hidden-warning {{'disallowedStaticMember2' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library}}
187+
} // namespace GlobalTest

0 commit comments

Comments
 (0)