Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2240233002:
Print additional message for RELEASE_ASSERT in builds used for fuzzing. (Closed)
DescriptionMake SECURITY_CHECK and SECURITY_DCHECK to print different messages.
RELEASE_ASSERT prints the same message as SECURITY_CHECK, because
RELEASE_ASSERT is deprecated and being actively replaced by SECURITY_CHECK.
BUG=620960
Committed: https://crrev.com/e2fd3f4dfb0e57e7086ca680bfe73c7cf290496d
Cr-Commit-Position: refs/heads/master@{#414684}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Move to #elif block (avoid changing debug builds) and use LOG_IF() instead of WTFLogAlways(). #
Total comments: 5
Patch Set 3 : Make SECURITY_CHECK and SECURITY_DCHECK different, use SECURITY_CHECK message in RELEASE_ASSERT. #
Total comments: 3
Patch Set 4 : #define RELEASE_ASSERT(condition) SECURITY_CHECK(condition) for our case. #
Messages
Total messages: 20 (6 generated)
inferno@chromium.org changed reviewers: + inferno@chromium.org, mbarbella@chromium.org, ochang@chromium.org, tkent@chromium.org
https://codereview.chromium.org/2240233002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/Assertions.h (right): https://codereview.chromium.org/2240233002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/Assertions.h:292: #if defined(ADDRESS_SANITIZER) This should come after #if ENABLE(ASSERT) block in an #elif (don't need an outer #if #endif. For debug builds, we don't want to change behavior. https://codereview.chromium.org/2240233002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/Assertions.h:294: WTFLogAlways("RELEASE_ASSERT failed.\n"); \ We would need to get condition failed and stacktrace. can you check if "LOG_IF(FATAL, !(condition)) << "Release assert failed: " #condition ". "" code does it. See also line 264 where it is used.
Thanks for comments Abhishek, your suggestion works. It gives the message + condition + stacktrace. But there is a compilation issue in text/StringConcatenate.h. I've uploaded a possible solution and commented about another one. Please take a look. https://codereview.chromium.org/2240233002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/text/StringConcatenate.h (right): https://codereview.chromium.org/2240233002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/text/StringConcatenate.h:174: void writeTo(LChar*) Another approach could be: NO_RETURN_DUE_TO_CRASH void writeTo(LChar*) { RELEASE_ASSERT(false); CRASH(); } Otherwise, the compiler doesn't believe that this function is 'noreturn' and gives a warning (i.e. error): ../../third_party/WebKit/Source/wtf/text/StringConcatenate.h:177:5: error: function declared 'noreturn' should not return [-Werror,-Winvalid-noreturn] } ^ 1 error generated.
An example of how a crash looks is available in ClusterFuzz CL: https://chromereviews.googleplex.com/496797013
https://codereview.chromium.org/2240233002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/Assertions.h (right): https://codereview.chromium.org/2240233002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/Assertions.h:267: #define RELEASE_ASSERT(condition) LOG_IF(FATAL, !(condition)) << "Release assert failed: " #condition ". " We deprecated RELEASE_ASSERT(), and are replacing RELEASE_ASSERT()s with SECURITY_CHECK(). Should we show the same log messages? SECURITY_CHECK() shows "Security check failed: " #condition ". " https://codereview.chromium.org/2240233002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/text/StringConcatenate.h (right): https://codereview.chromium.org/2240233002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/text/StringConcatenate.h:174: void writeTo(LChar*) On 2016/08/25 at 10:45:53, mmoroz wrote: > Another approach could be: > > NO_RETURN_DUE_TO_CRASH void writeTo(LChar*) > { > RELEASE_ASSERT(false); > CRASH(); > } > > > Otherwise, the compiler doesn't believe that this function is 'noreturn' and gives a warning (i.e. error): > > ../../third_party/WebKit/Source/wtf/text/StringConcatenate.h:177:5: error: function declared 'noreturn' should not return [-Werror,-Winvalid-noreturn] > } > ^ > 1 error generated. IMO, it's ok to remove NO_RETURN_DE_TO_CRASH here. NO_RETURN_DE_TO_CRASH doesn't have much benefit, and I'd like to remove NO_RETURN_DE_TO_CRASH itself.
https://codereview.chromium.org/2240233002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/Assertions.h (right): https://codereview.chromium.org/2240233002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/Assertions.h:267: #define RELEASE_ASSERT(condition) LOG_IF(FATAL, !(condition)) << "Release assert failed: " #condition ". " On 2016/08/25 12:54:06, tkent wrote: > We deprecated RELEASE_ASSERT(), and are replacing RELEASE_ASSERT()s with > SECURITY_CHECK(). Should we show the same log messages? SECURITY_CHECK() shows > "Security check failed: " #condition ". " Good point! +1 to replace 'Release assert' -> 'Security check'
https://codereview.chromium.org/2240233002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/Assertions.h (right): https://codereview.chromium.org/2240233002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/Assertions.h:267: #define RELEASE_ASSERT(condition) LOG_IF(FATAL, !(condition)) << "Release assert failed: " #condition ". " On 2016/08/25 13:13:45, mmoroz wrote: > On 2016/08/25 12:54:06, tkent wrote: > > We deprecated RELEASE_ASSERT(), and are replacing RELEASE_ASSERT()s with > > SECURITY_CHECK(). Should we show the same log messages? SECURITY_CHECK() > shows > > "Security check failed: " #condition ". " > > Good point! +1 to replace 'Release assert' -> 'Security check' Hm, on ClusterFuzz side we mark 'Security CHECK failure' as a security issue, because it is intended to be a Debug-only check. Need to distinguish between SECURITY_CHECK and SECURITY_DCHECK. You previously raised this question at line 250 as well.
https://codereview.chromium.org/2240233002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/Assertions.h (right): https://codereview.chromium.org/2240233002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/Assertions.h:249: #define SECURITY_DCHECK(condition) LOG_IF(FATAL, !(condition)) << "Security DCHECK failed: " #condition ". " After the discussion in previous patchset, I propose two different messages for SECURITY_CHECK and SECURITY_DCHECK. On CF side we will mark 'Security DCHECK failed' as a security issue, 'Security CHECK failed' as a not security issue. RELEASE_ASSERT uses the same message to simplify its deprecation process: CF shouldn't break if we replace RELEASE_ASSERT()s with SECURITY_CHECK()s.
lgtm
lgtm with nits. https://codereview.chromium.org/2240233002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/Assertions.h (right): https://codereview.chromium.org/2240233002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/Assertions.h:266: #define RELEASE_ASSERT(condition) LOG_IF(FATAL, !(condition)) << "Security CHECK failed: " #condition ". " Why not #define RELEASE_ASSERT(condition) SECURITY_CHECK(condition)
https://codereview.chromium.org/2240233002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/Assertions.h (right): https://codereview.chromium.org/2240233002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/Assertions.h:266: #define RELEASE_ASSERT(condition) LOG_IF(FATAL, !(condition)) << "Security CHECK failed: " #condition ". " On 2016/08/25 15:03:06, inferno wrote: > Why not #define RELEASE_ASSERT(condition) SECURITY_CHECK(condition) Oh, yes! Thanks.
Description was changed from ========== Print additional message for RELEASE_ASSERT in builds used for fuzzing. BUG=620960 ========== to ========== Make SECURITY_CHECK and SECURITY_DCHECK to print different messages. RELEASE_ASSERT prints the same message as SECURITY_CHECK, because RELEASE_ASSERT is deprecated and being actively replaced by SECURITY_CHECK. BUG=620960 ==========
The CQ bit was checked by mmoroz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org, inferno@chromium.org Link to the patchset: https://codereview.chromium.org/2240233002/#ps60001 (title: "#define RELEASE_ASSERT(condition) SECURITY_CHECK(condition) for our case.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Make SECURITY_CHECK and SECURITY_DCHECK to print different messages. RELEASE_ASSERT prints the same message as SECURITY_CHECK, because RELEASE_ASSERT is deprecated and being actively replaced by SECURITY_CHECK. BUG=620960 ========== to ========== Make SECURITY_CHECK and SECURITY_DCHECK to print different messages. RELEASE_ASSERT prints the same message as SECURITY_CHECK, because RELEASE_ASSERT is deprecated and being actively replaced by SECURITY_CHECK. BUG=620960 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Make SECURITY_CHECK and SECURITY_DCHECK to print different messages. RELEASE_ASSERT prints the same message as SECURITY_CHECK, because RELEASE_ASSERT is deprecated and being actively replaced by SECURITY_CHECK. BUG=620960 ========== to ========== Make SECURITY_CHECK and SECURITY_DCHECK to print different messages. RELEASE_ASSERT prints the same message as SECURITY_CHECK, because RELEASE_ASSERT is deprecated and being actively replaced by SECURITY_CHECK. BUG=620960 Committed: https://crrev.com/e2fd3f4dfb0e57e7086ca680bfe73c7cf290496d Cr-Commit-Position: refs/heads/master@{#414684} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e2fd3f4dfb0e57e7086ca680bfe73c7cf290496d Cr-Commit-Position: refs/heads/master@{#414684} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
