Bug 78513 - If memset gets inlined at WebKit/Source/JavaScriptCore/wtf/Vector.h, it may produce a warning
Summary: If memset gets inlined at WebKit/Source/JavaScriptCore/wtf/Vector.h, it may p...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-13 11:51 PST by asharif.tools
Modified: 2012-02-17 23:28 PST (History)
6 users (show)

See Also:


Attachments
Fixes the problem. (1.09 KB, patch)
2012-02-13 14:32 PST, asharif.tools
ap: review-
ap: commit-queue-
Details | Formatted Diff | Diff
Should not have any runtime overhead with this patch at -O2. (1.18 KB, patch)
2012-02-13 20:26 PST, asharif.tools
no flags Details | Formatted Diff | Diff
Fixed style warnings. (1.18 KB, patch)
2012-02-13 20:48 PST, asharif.tools
ap: commit-queue-
Details | Formatted Diff | Diff
Fixed line-wrap issue. (1.16 KB, patch)
2012-02-14 10:15 PST, asharif.tools
no flags Details | Formatted Diff | Diff
Proposed fix. (1.16 KB, patch)
2012-02-15 20:33 PST, asharif.tools
no flags Details | Formatted Diff | Diff
Proposed fix. (1.15 KB, patch)
2012-02-17 13:34 PST, asharif.tools
ap: review+
ap: commit-queue-
Details | Formatted Diff | Diff
Proposed fix. (1.37 KB, patch)
2012-02-17 14:23 PST, asharif.tools
ap: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Proposed fix. (1.41 KB, patch)
2012-02-17 17:56 PST, asharif.tools
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description asharif.tools 2012-02-13 11:51:50 PST
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);
        }
    };
Comment 1 asharif.tools 2012-02-13 14:32:33 PST
Created attachment 126836 [details]
Fixes the problem.
Comment 2 Nico Weber 2012-02-13 15:04:03 PST
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 3 Alexey Proskuryakov 2012-02-13 17:04:10 PST
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.
Comment 4 asharif.tools 2012-02-13 20:26:55 PST
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.
Comment 5 WebKit Review Bot 2012-02-13 20:29:38 PST
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.
Comment 6 asharif.tools 2012-02-13 20:48:09 PST
Created attachment 126901 [details]
Fixed style warnings.
Comment 7 Alexey Proskuryakov 2012-02-14 10:13:56 PST
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.
Comment 8 asharif.tools 2012-02-14 10:15:46 PST
Created attachment 126995 [details]
Fixed line-wrap issue.
Comment 9 WebKit Review Bot 2012-02-15 01:15:45 PST
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.
Comment 10 asharif.tools 2012-02-15 20:33:38 PST
Created attachment 127304 [details]
Proposed fix.

I rebased this patch.
Comment 11 asharif.tools 2012-02-17 10:02:39 PST
Ping?

Any updates on this? If this solution is not acceptable, let me know so we can close this bug.
Comment 12 Alexey Proskuryakov 2012-02-17 10:21:05 PST
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.
Comment 13 Mike Frysinger 2012-02-17 10:44:38 PST
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.
Comment 14 Alexey Proskuryakov 2012-02-17 13:18:58 PST
> 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).
Comment 15 asharif.tools 2012-02-17 13:34:18 PST
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 16 Alexey Proskuryakov 2012-02-17 14:17:55 PST
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)
Comment 17 Alexey Proskuryakov 2012-02-17 14:18:21 PST
Oh, and please mention Fortify in ChangeLog.
Comment 18 asharif.tools 2012-02-17 14:23:02 PST
Created attachment 127659 [details]
Proposed fix.

Addressed reviewers' comments.
Comment 19 Alexey Proskuryakov 2012-02-17 14:29:58 PST
Comment on attachment 127659 [details]
Proposed fix.

Thank you!
Comment 20 WebKit Review Bot 2012-02-17 15:29:53 PST
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
Comment 21 Alexey Proskuryakov 2012-02-17 16:57:38 PST
I don't understand why the commit queue freaked out. Adam, Eric, do you see what's wrong?
Comment 22 Eric Seidel (no email) 2012-02-17 17:54:35 PST
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
Comment 23 Eric Seidel (no email) 2012-02-17 17:55:07 PST
Our OOPS! convention in WebKit is not very discoverable.  I think we should consider changing it to DO_NOT_COMMIT or similar.
Comment 24 asharif.tools 2012-02-17 17:56:59 PST
Created attachment 127682 [details]
Proposed fix.

Rebased patch. Added exclamation point after OOPS.
Comment 25 WebKit Review Bot 2012-02-17 23:28:38 PST
Comment on attachment 127682 [details]
Proposed fix.

Clearing flags on attachment: 127682

Committed r108153: <http://trac.webkit.org/changeset/108153>
Comment 26 WebKit Review Bot 2012-02-17 23:28:44 PST
All reviewed patches have been landed.  Closing bug.