Bug 78513 – If memset gets inlined at WebKit/Source/JavaScriptCore/wtf/Vector.h, it may produce a warning

• Component: JavaScriptCore
• Status: RESOLVED FIXED

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.

[Nico Weber]

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.

[Alexey Proskuryakov]

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.

[WebKit Review Bot]

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.

[Alexey Proskuryakov]

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.

[WebKit Review Bot]

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.

[Alexey Proskuryakov]

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.

[Mike Frysinger]

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.

[Alexey Proskuryakov]

> 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.

[Alexey Proskuryakov]

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)

[Alexey Proskuryakov]

Oh, and please mention Fortify in ChangeLog.

Created attachment 127659 [details]
Proposed fix.

Addressed reviewers' comments.

[Alexey Proskuryakov]

Comment on attachment 127659 [details]
Proposed fix.

Thank you!

[WebKit Review Bot]

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

[Alexey Proskuryakov]

I don't understand why the commit queue freaked out. Adam, Eric, do you see what's wrong?

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[WebKit Review Bot]

Comment on attachment 127682 [details]
Proposed fix.

Clearing flags on attachment: 127682

Committed r108153: <http://trac.webkit.org/changeset/108153>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.