This warning happens in the following conditions: 1. If chain of calls is inlined, including memset. 2. memset has a fill value of non-0, but a size of 0. 3. -D_FORTIFY_SOURCE=2 was passed. This is the warning that I see when building Chrome. In file included from /build/amd64-generic/usr/include/string.h:640:0, from third_party/WebKit/Source/JavaScriptCore/wtf/FastAllocBase.h:90, from third_party/WebKit/Source/JavaScriptCore/wtf/Vector.h:25, from third_party/WebKit/Source/JavaScriptCore/wtf/text/StringImpl.h:31, from third_party/WebKit/Source/JavaScriptCore/wtf/text/WTFString.h:29, from third_party/WebKit/Source/WebCore/platform/text/PlatformString.h:28, from third_party/WebKit/Source/WebCore/loader/cache/CachedResource.h:28, from third_party/WebKit/Source/WebCore/loader/cache/CachedImage.h:26, from third_party/WebKit/Source/WebCore/rendering/RenderObject.h:29, from third_party/WebKit/Source/WebCore/rendering/RenderBoxModelObject.h:27, from third_party/WebKit/Source/WebCore/rendering/RenderBox.h:26, from third_party/WebKit/Source/WebCore/rendering/RenderFrameSet.h:26, from third_party/WebKit/Source/WebCore/rendering/RenderFrameSet.cpp:25: In function 'void* memset(void*, int, size_t)', inlined from 'WebCore::FrameEdgeInfo WebCore::RenderFrameSet::edgeInfo() const' at third_party/WebKit/Source/JavaScriptCore/wtf/Vector.h:184:13, inlined from 'void WebCore::RenderFrameSet::computeEdgeInfo()' at third_party/WebKit/Source/WebCore/rendering/RenderFrameSet.cpp:430:62: /build/amd64-generic/usr/include/bits/string3.h:82:35: error: call to '__warn_memset_zero_len' declared with attribute warning: memset used with constant zero length parameter; this could be due to transposed parameters [-Werror] cc1plus: all warnings being treated as errors This can be fixed with the following patch: diff --git a/Source/JavaScriptCore/wtf/Vector.h b/Source/JavaScriptCore/wtf/Vector.h index 175f1a5..bf15998 100644 --- a/Source/JavaScriptCore/wtf/Vector.h +++ b/Source/JavaScriptCore/wtf/Vector.h @@ -181,7 +181,8 @@ namespace WTF { static void uninitializedFill(T* dst, T* dstEnd, const T& val) { ASSERT(sizeof(T) == sizeof(char)); - memset(dst, val, dstEnd - dst); + if (dstEnd - dst) + memset(dst, val, dstEnd - dst); } };
Created attachment 126836 [details] Fixes the problem.
You usually upload patches with "Reviewed by NOBODY (OOPS)", and the scripts put in a name after someone reviewed this. I'm not a webkit reviewer, so I can't review this. I also don't think this should be done, the error sounds like a toolchain bug.
Comment on attachment 126836 [details] Fixes the problem. This makes uninitializedFill slower for the benefit of a platform specific tool. There must be a way to shut down the warning that doesn't have runtime costs.
Created attachment 126899 [details] Should not have any runtime overhead with this patch at -O2. This patch is now checking if dstEnd - dst is a constant before doing the 0 check. If dstEnd - dst is not a constant, the first part of the if condition is true and the whole if condition should be folded away.
Attachment 126899 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/wtf/Vector.h:185: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/JavaScriptCore/wtf/Vector.h:185: Extra space for operator ! [whitespace/operators] [4] Source/JavaScriptCore/wtf/Vector.h:186: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 3 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 126901 [details] Fixed style warnings.
Comment on attachment 126901 [details] Fixed style warnings. View in context: https://bugs.webkit.org/attachment.cgi?id=126901&action=review This might be OK, but the whole warning seems like something that should be just globally disabled somehow. > Source/JavaScriptCore/wtf/Vector.h:186 > + if (!__builtin_constant_p(dstEnd - dst) > + || (!(dstEnd - dst))) Please don't wrap this line.
Created attachment 126995 [details] Fixed line-wrap issue.
Attachment 126995 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource From git://git.webkit.org/WebKit + a9730c4...fb50dbd master -> origin/master (forced update) First, rewinding head to replay your work on top of it... Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets Using index info to reconstruct a base tree... <stdin>:1578: trailing whitespace. <stdin>:1647: trailing whitespace. <stdin>:1657: trailing whitespace. <stdin>:1672: trailing whitespace. return 0; <stdin>:1674: trailing whitespace. warning: squelched 7 whitespace errors warning: 12 lines add whitespace errors. Falling back to patching base and 3-way merge... warning: too many files (created: 168753 deleted: 3), skipping inexact rename detection Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/wk2/Skipped Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Auto-merging Source/WebCore/css/CSSCalculationValue.cpp Auto-merging Source/WebCore/css/CSSCalculationValue.h Auto-merging Source/WebCore/css/CSSParser.cpp Auto-merging Source/WebKit/mac/ChangeLog CONFLICT (content): Merge conflict in Source/WebKit/mac/ChangeLog Auto-merging Source/WebKit2/ChangeLog CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog Auto-merging Tools/ChangeLog CONFLICT (content): Merge conflict in Tools/ChangeLog Failed to merge in the changes. Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 127304 [details] Proposed fix. I rebased this patch.
Ping? Any updates on this? If this solution is not acceptable, let me know so we can close this bug.
One question is whether there is a less intrusive way to disable this warning. A quick web search reveals that "With -D_FORTIFY_SOURCE=2 ... some conforming programs might fail." <http://gcc.gnu.org/ml/gcc-patches/2004-09/msg02055.html>. So, I'm not sure if we want to subscribe to supporting this mode. A discussion on webkit-dev mailing list may clarify things.
while it's true that fortify breaks code that is correct from the language/runtime pov, we're talking about snippets that arguably go against style tastes. such as: struct { char a[2]; char b; char d[3]; } foo; strcpy(&foo.a[0], "aabdd"); many distros now build with fortify enabled by default for pretty much all packages (Debian/Ubuntu/Fedora/Redhat/Gentoo). it has certainly been a much bigger benefit than the handful of downsides. the proposed patch probably should have a comment in there explaining why this magic check is active. or perhaps replace/augment the GCC define with _FORTIFY_SOURCE so that the code is self-documenting.
> replace/augment the GCC define with _FORTIFY_SOURCE so that the code is self-documenting I like this idea (augmenting would likely be more self-documenting).
Created attachment 127643 [details] Proposed fix. Added ifdef _FORTIFY_SOURCE around the patch. I kept the COMPILER(GCC) because __builtin_constant_p() may not be available on other compilers.
Comment on attachment 127643 [details] Proposed fix. I think that this is OK to land, but please merge the #ifs in one line: #if COMPILER(GCC) && defined(_FORTIFY_SOURCE)
Oh, and please mention Fortify in ChangeLog.
Created attachment 127659 [details] Proposed fix. Addressed reviewers' comments.
Comment on attachment 127659 [details] Proposed fix. Thank you!
Comment on attachment 127659 [details] Proposed fix. Rejecting attachment 127659 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/JavaScriptCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/11543290
I don't understand why the commit queue freaked out. Adam, Eric, do you see what's wrong?
Comment on attachment 127659 [details] Proposed fix. View in context: https://bugs.webkit.org/attachment.cgi?id=127659&action=review > Source/JavaScriptCore/ChangeLog:10 > + Reviewed by NOBODY (OOPS). I believe it normally has a ! after the OOPS. http://trac.webkit.org/browser/trunk/Tools/Scripts/VCSUtils.pm#L1399
Our OOPS! convention in WebKit is not very discoverable. I think we should consider changing it to DO_NOT_COMMIT or similar.
Created attachment 127682 [details] Proposed fix. Rebased patch. Added exclamation point after OOPS.
Comment on attachment 127682 [details] Proposed fix. Clearing flags on attachment: 127682 Committed r108153: <http://trac.webkit.org/changeset/108153>
All reviewed patches have been landed. Closing bug.