Skip to content

[analyzer] Don't invalidate the super region when a std object ctor runs #100405

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

Merged
merged 1 commit into from
Jul 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions clang/lib/StaticAnalyzer/Core/CallEvent.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -923,12 +923,31 @@ SVal AnyCXXConstructorCall::getCXXThisVal() const {
return UnknownVal();
}

static bool isWithinStdNamespace(const Decl *D) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this function could be useful for other checkers as well; consider moving this to an externally visible location (e.g. turn this into a method of Decl).

Copy link
Contributor Author

@steakhal steakhal Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I think I'd prefer moving this utility in a followup patch, as it may take a while to reach consensus where to put it. And I don't want to block this PR until that happens.
Would it be okay with you? @NagyDonat

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, feel free to leave it for a followup patch.

Copy link
Contributor Author

@steakhal steakhal Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AaronBallman Do you think in clang we already have something like this? Or it would make sense to have this part of Decl?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've already got one of these at home:

bool isInStdNamespace() const;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK not quite. Mine works even if the class a nested subclass within the std namespace, such as "std::vector::iterator". So directly using isInStdNamespace wouldn't quite cut it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, that behavior would be surprising to me because nested subclasses are not in the std namespace. (Not saying it's wrong for the static analyzer, but I would not expect that behavior when doing code reviews, for example.)

const DeclContext *DC = D->getDeclContext();
while (DC) {
if (const auto *NS = dyn_cast<NamespaceDecl>(DC);
NS && NS->isStdNamespace())
return true;
DC = DC->getParent();
}
return false;
}

void AnyCXXConstructorCall::getExtraInvalidatedValues(ValueList &Values,
RegionAndSymbolInvalidationTraits *ETraits) const {
SVal V = getCXXThisVal();
if (SymbolRef Sym = V.getAsSymbol(true))
ETraits->setTrait(Sym,
RegionAndSymbolInvalidationTraits::TK_SuppressEscape);

// Standard classes don't reinterpret-cast and modify super regions.
const bool IsStdClassCtor = isWithinStdNamespace(getDecl());
if (const MemRegion *Obj = V.getAsRegion(); Obj && IsStdClassCtor) {
ETraits->setTrait(
Obj, RegionAndSymbolInvalidationTraits::TK_DoNotInvalidateSuperRegion);
}

Values.push_back(V);
}

Expand Down
115 changes: 115 additions & 0 deletions clang/test/Analysis/call-invalidation.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s

template <class T> void clang_analyzer_dump(T);
void clang_analyzer_eval(bool);

void usePointer(int * const *);
Expand Down Expand Up @@ -165,3 +166,117 @@ void testMixedConstNonConstCalls() {
useFirstNonConstSecondConst(&(s2.x), &(s2.y));
clang_analyzer_eval(s2.y == 1); // expected-warning{{UNKNOWN}}
}

namespace std {
class Opaque {
public:
Opaque();
int nested_member;
};
} // namespace std

struct StdWrappingOpaque {
std::Opaque o; // first member
int uninit;
};
struct StdWrappingOpaqueSwapped {
int uninit; // first member
std::Opaque o;
};

int testStdCtorDoesNotInvalidateParentObject() {
StdWrappingOpaque obj;
int x = obj.o.nested_member; // no-garbage: std::Opaque::ctor might initialized this
int y = obj.uninit; // FIXME: We should have a garbage read here. Read the details.
// As the first member ("obj.o") is invalidated, a conjured default binding is bound
// to the offset 0 within cluster "obj", and this masks every uninitialized fields
// that follows. We need a better store with extents to fix this.
return x + y;
}

int testStdCtorDoesNotInvalidateParentObjectSwapped() {
StdWrappingOpaqueSwapped obj;
int x = obj.o.nested_member; // no-garbage: std::Opaque::ctor might initialized this
int y = obj.uninit; // expected-warning {{Assigned value is garbage or undefined}}
return x + y;
}

class UserProvidedOpaque {
public:
UserProvidedOpaque(); // might reinterpret_cast(this)
int nested_member;
};

struct WrappingUserProvidedOpaque {
UserProvidedOpaque o; // first member
int uninit;
};
struct WrappingUserProvidedOpaqueSwapped {
int uninit; // first member
UserProvidedOpaque o;
};

int testUserProvidedCtorInvalidatesParentObject() {
WrappingUserProvidedOpaque obj;
int x = obj.o.nested_member; // no-garbage: UserProvidedOpaque::ctor might initialized this
int y = obj.uninit; // no-garbage: UserProvidedOpaque::ctor might reinterpret_cast(this) and write to the "uninit" member.
return x + y;
}

int testUserProvidedCtorInvalidatesParentObjectSwapped() {
WrappingUserProvidedOpaqueSwapped obj;
int x = obj.o.nested_member; // no-garbage: same as above
int y = obj.uninit; // no-garbage: same as above
return x + y;
}

struct WrappingStdWrappingOpaqueOuterInits {
int first = 1;
std::Opaque second;
int third = 3;
WrappingStdWrappingOpaqueOuterInits() {
clang_analyzer_dump(first); // expected-warning {{1 S32b}}
clang_analyzer_dump(second.nested_member); // expected-warning {{derived_}}
clang_analyzer_dump(third); // expected-warning {{3 S32b}}
}
};

struct WrappingUserProvidedOpaqueOuterInits {
int first = 1; // Potentially overwritten by UserProvidedOpaque::ctor
UserProvidedOpaque second; // Invalidates the object so far.
int third = 3; // Happens after UserProvidedOpaque::ctor, thus preserved!
WrappingUserProvidedOpaqueOuterInits() {
clang_analyzer_dump(first); // expected-warning {{derived_}}
clang_analyzer_dump(second.nested_member); // expected-warning {{derived_}}
clang_analyzer_dump(third); // expected-warning {{3 S32b}}
}
};

extern "C++" {
namespace std {
inline namespace v1 {
namespace custom_ranges {
struct Fancy {
struct iterator {
struct Opaque {
Opaque();
int nested_member;
}; // struct Opaque
}; // struct iterator
}; // struct Fancy
} // namespace custom_ranges
} // namespace v1
} // namespace std
} // extern "C++"

struct StdWrappingFancyOpaque {
int uninit;
std::custom_ranges::Fancy::iterator::Opaque o;
};

int testNestedStdNamespacesAndRecords() {
StdWrappingFancyOpaque obj;
int x = obj.o.nested_member; // no-garbage: ctor
int y = obj.uninit; // expected-warning {{Assigned value is garbage or undefined}}
return x + y;
}
Loading