Suppress more clang static analyzer warnings in WTF #38038
Conversation
rniwa
added
the
New Bugs
|
EWS run on previous version of this PR (hash 9aae735) Details |
rniwa
force-pushed
the
fix284783-suppress-wtf-warnings
branch
from
9aae735 to
d2004bc
Compare
|
EWS run on previous version of this PR (hash d2004bc) Details |
Source/WTF/wtf/text/AtomStringImpl.h
Outdated
There was a problem hiding this comment.
Should this if check use a dynamicDowncast? once AtomStringImpl supports it?
There was a problem hiding this comment.
I don't think there is a much value in using dynamicDowncast here since AtomStringImpl is a phantom type. We never directly create AtomStringImpl. We just cast StringImpl to AtomStringImpl when isAtom().
There was a problem hiding this comment.
Let me put it some other way: Couldn't we get security bugs if someone did an incorrect cast from StringImpl to AtomStringImpl even though isAtom() returns false? If the answer is yes, I think this is a clear case for using downcast / dynamicDowncast (cc @geoffreygaren)
There was a problem hiding this comment.
I don't think so. The memory layout of AtomStringImpl and StringImpl are identical. The only difference is whether the string belongs to the atom string table or not. Of course, if you treat a string not in the atom string table as it's in the table or vice versa, it would be problematic but that's independent of whether we cast StringImpl to AtomStringImpl or not.
There was a problem hiding this comment.
There are already isAtom() checks, which seems to nicely map to the dynamicCast model, I don't understand why we wouldn't want to catch bad casts in release builds and make the code consistent with the rest of the code base. It doesn't seem like a lot of work and I don't see the downside.
There was a problem hiding this comment.
I think @rniwa has it mostly right. I agree that this is a downcast situation but not one that involves the classic risks of type confusion. With other such situations the objects have different sizes so type confusion can be exploited to read or write past the end of an object. In this situation the implications are far less dire. Having an AtomStringImpl pointing to something not an atom might be exploitable in some way but it would have to be more of a logic mistake and I can't immediately think of anything.
On the other hand if we can afford it from a performance point of view it seems fine to use downcast and dynamicDowncast anyway. Doesn't seem to be harmful to recheck the isAtom bit except for the performance cost.
There was a problem hiding this comment.
To be clear, there may be places where downcast<>() has extra cost but for this particular piece of code, using dynamicDowncast<>() would have no extra cost since this original code is already checking isAtom. We'd replace isAtom check + static_cast with dynamicDowncast which is identical perf-wise but is more concise and modern.
There was a problem hiding this comment.
For analogy, there are plenty of cases where holding a smart pointer on the stack is not strictly needed for security because the developer can reason about the code and claim that it is safe without that smart pointer. Yet, we still expect people to follow the pattern to silence static analysis warnings and guarantee safety, unless there is a very good reason NOT to do so.
Seems similar here, there is no safety issue but the static analysis wants use to use downcast / dynamicDowncast. While there is no safety issue, I think downcast / dynamicDowncast can still help catch logic errors so I believe it does provide some value. It would also make things consistent with the rest of our code base. I personally don't see a downside to using downcast / dynamicDowncast. What is the good reason NOT to use downcast?
Darin mentioned perf. That could be a reason. I don't think it applies to this case here because it was already doing a runtime type check. The check would just move inside the dynamicDowncast. There could be other places where we use static_cast<>() without a type check and downcast would add a cost. In those cases, I still think we should use downcast as long as it doesn't regress perf. If it does regress perf, we can static_cast with a comment explaining why we're not using downcast.
There was a problem hiding this comment.
I agree with the analysis here that explains why it's not a memory safety error to mis-cast StringImpl to AtomStringImpl.
But our goal with Safer C++ is not to require expert exegesis. We want our code to be self-evidently safe based on idiomatic and automatically verified language. So I favor doing the dynamicDowncast<> here.
There was a problem hiding this comment.
My request at this point: Use dynamicDowncast as an elegant way to write exactly the same code in a way that is understood by our safety tools. I don't think we should overstate that benefit — our discussion here is simplifying some things in a possibly misleading way — but it is a real one.
Source/WTF/wtf/text/AtomStringImpl.h
Outdated
Source/WTF/wtf/text/AtomStringImpl.h
Outdated
Source/WTF/wtf/text/AtomStringImpl.h
Outdated
rniwa
force-pushed
the
fix284783-suppress-wtf-warnings
branch
from
d2004bc to
d623d20
Compare
|
EWS run on previous version of this PR (hash d623d20) Details |
Safer C++ Build #14069 (d2004bc)❌ Found 1 new failure. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming.
|
Safer C++ Build #14142 (d623d20)❌ Found 1 new failure. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming.
|
rniwa
force-pushed
the
fix284783-suppress-wtf-warnings
branch
from
d623d20 to
899ec4c
Compare
|
EWS run on previous version of this PR (hash 899ec4c) Details |
Source/WTF/wtf/text/AtomStringImpl.h
Outdated
There was a problem hiding this comment.
Ugh... I keep mixing up the star position in WebKit & LLVM.
There was a problem hiding this comment.
I would have used the name atom without the impl suffix here and elsewhere.
rniwa
force-pushed
the
fix284783-suppress-wtf-warnings
branch
2 times, most recently
from
fdc65b5 to
ce4ba45
Compare
|
EWS run on current version of this PR (hash ce4ba45) Details |
|
EWS run on previous version of this PR (hash fdc65b5) Details |
webkit-ews-buildbot
added
the
merging-blocked
rniwa
added
unsafe-merge-queue
https://bugs.webkit.org/show_bug.cgi?id=284783 Reviewed by Chris Dumez. Suppressed more clang static analyzer warnings. * Source/WTF/SaferCPPExpectations/MemoryUnsafeCastCheckerExpectations: * Source/WTF/SaferCPPExpectations/UncountedCallArgsCheckerExpectations: * Source/WTF/SaferCPPExpectations/UncountedLocalVarsCheckerExpectations: * Source/WTF/wtf/text/AtomString.h: (WTF::nullAtom): (WTF::emptyAtom): * Source/WTF/wtf/text/AtomStringImpl.h: (WTF::AtomStringImpl::lookUp): (WTF::AtomStringImpl::add): (isType): * Source/WTF/wtf/text/StringImpl.h: (WTF::StringImpl::empty): (WTF::StringImpl::createSubstringSharingImpl): (WTF::StringImpl::tryCreateUninitialized): (WTF::StringImpl::StaticStringImpl::operator StringImpl&): * Source/WTF/wtf/text/StringView.h: (WTF::String::hasInfixStartingAt const): (WTF::String::hasInfixEndingAt const): * Source/WTF/wtf/text/WTFString.h: (WTF::nullString): (WTF::emptyString): (WTF::String::defaultWritingDirection const): (WTF::String::substring const): (WTF::String::operator NSString * const): (WTF::String::removeCharacters const): * Source/WebKit/UIProcess/API/Cocoa/WKWebExtensionCommand.mm: (-[WKWebExtensionCommand activationKey]): Canonical link: https://commits.webkit.org/288214@main
webkit-commit-queue
force-pushed
the
fix284783-suppress-wtf-warnings
branch
from
ce4ba45 to
0477b9f
Compare
|
Committed 288214@main (0477b9f): https://commits.webkit.org/288214@main Reviewed commits have been landed. Closing PR #38038 and removing active labels. |
webkit-commit-queue
removed
the
unsafe-merge-queue
7 participants
0477b9f
ce4ba45
🧪 api-ios🛠 playstation