Bug 52585 - REGRESSION(r75709): Return value of fscanf() shouldn't be ignored.
Summary: REGRESSION(r75709): Return value of fscanf() shouldn't be ignored.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P1 Blocker
Assignee: Nobody
URL:
Keywords: Qt
Depends on:
Blocks: 45384
  Show dependency treegraph
 
Reported: 2011-01-17 09:44 PST by Jarred Nicholls
Modified: 2011-01-17 19:10 PST (History)
4 users (show)

See Also:


Attachments
Proposed patch (1.32 KB, patch)
2011-01-17 10:02 PST, Jarred Nicholls
ossy: review-
Details | Formatted Diff | Diff
Proposed patch (1.66 KB, patch)
2011-01-17 13:13 PST, Jarred Nicholls
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
Proposed patch (1.50 KB, patch)
2011-01-17 13:24 PST, Jarred Nicholls
jarred: review-
Details | Formatted Diff | Diff
Proposed patch (1.50 KB, patch)
2011-01-17 13:26 PST, Jarred Nicholls
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jarred Nicholls 2011-01-17 09:44:22 PST
We have a broken build w/ gcc 4.4.5 and libc 2.12.1 on Ubuntu 10.10 due to a warn_unused_result attribute applied on fscanf, and a misuse in change set 75709 (http://trac.webkit.org/changeset/75709/trunk/Source/JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp) where the return value is not checked/used.  Patch to come shortly.
Comment 1 Jarred Nicholls 2011-01-17 10:02:13 PST
Created attachment 79183 [details]
Proposed patch

Added another compiler flag to ignore unused return values, and to not treat those warnings as errors.  Also removed duplicate -Wreturn-type entry.
Comment 2 Csaba Osztrogonác 2011-01-17 12:35:50 PST
It isn't Qt specific bug, but the return value of fscanf() should be always checked to avoid strange bugs. I prefer check the return value of fscanf() to paper over the problem with -Wreturn-type compiler option.
Comment 3 Csaba Osztrogonác 2011-01-17 12:38:29 PST
Comment on attachment 79183 [details]
Proposed patch

r- due to my last comment.

And rs=me to remove the redundant -Wreturn-type.
Comment 4 Jarred Nicholls 2011-01-17 12:48:02 PST
(In reply to comment #2)
> It isn't Qt specific bug, but the return value of fscanf() should be always checked to avoid strange bugs. I prefer check the return value of fscanf() to paper over the problem with -Wreturn-type compiler option.

That was my original patch :) I know it's not Qt specific regression, but it is (at least) effecting the Qt Linux build.  It got past the build bots somehow ;)  I'll revise the patch.
Comment 5 Csaba Osztrogonác 2011-01-17 12:49:19 PST
452	static void maybeModifyVMPoolSize() 
453	{ 
454	    FILE* fp = fopen("/proc/sys/vm/overcommit_memory", "r"); 
455	    if (!fp) 
456	        return; 
457	 
458	    unsigned overcommit = 0; 
459	    fscanf(fp, "%u", &overcommit); 

Here the return value of fscanf must be true, so we 
can ignore it here, but we shouldn't do it in general.

I think the similar "check" to http://trac.webkit.org/changeset/66984 would be good:


+ // We have to use the return value of fwrite to avoid "ignoring return value" gcc warning
+ // See https://bugs.webkit.org/show_bug.cgi?id=45384 for details. 
- fscanf(fp, "%u", &overcommit); 
+ if (fscanf(fp, "%u", &overcommit)) {}
Comment 6 Csaba Osztrogonác 2011-01-17 12:51:09 PST
Once we should find a general solution for https://bugs.webkit.org/show_bug.cgi?id=45384 ... Any volunteer? :)
Comment 7 Jarred Nicholls 2011-01-17 13:13:42 PST
Created attachment 79209 [details]
Proposed patch

Check the return value instead of disabling a gcc warning flag, to benefit all ports.
Comment 8 Csaba Osztrogonác 2011-01-17 13:14:48 PST
Comment on attachment 79209 [details]
Proposed patch

LGTM, r=me.
Comment 9 Darin Adler 2011-01-17 13:17:33 PST
Comment on attachment 79209 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79209&action=review

> Source/JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:466
> +    // We have to use the return value of fscanf to avoid "ignoring return value" gcc warning
> +    // See https://bugs.webkit.org/show_bug.cgi?id=45384 for details.

This comment isn’t needed. We need to check the return value of fscanf because that’s how you use fscanf! I don’t think we need a reference to this bug.

> Source/JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:467
> +    if (fscanf(fp, "%u", &overcommit)) {

This is not right. We need to check if the value is 1 rather than checking for 0. This function can return EOF, for example. Since we’re going to the trouble to check here, I suggest doing the check correctly.
Comment 10 Jarred Nicholls 2011-01-17 13:19:54 PST
Spot on Darin, will revise.
Comment 11 Jarred Nicholls 2011-01-17 13:24:16 PST
Created attachment 79210 [details]
Proposed patch

explicitly check for 1, as checking for "anything but zero" is incorrect as EOF could be returned.
Comment 12 Jarred Nicholls 2011-01-17 13:26:28 PST
Created attachment 79211 [details]
Proposed patch

broken patch
Comment 13 Csaba Osztrogonác 2011-01-17 13:44:22 PST
Comment on attachment 79211 [details]
Proposed patch

r=me, based on Darin's comment. Thanks for the advice, this patch is better.
Comment 14 Csaba Osztrogonác 2011-01-17 13:46:46 PST
Jarred, if you realized that you upload a bad patch next time, just 
remove r? flag and set obsolete flag instead of giving a "self-r-" .
Comment 15 Jarred Nicholls 2011-01-17 13:48:29 PST
(In reply to comment #14)
> Jarred, if you realized that you upload a bad patch next time, just 
> remove r? flag and set obsolete flag instead of giving a "self-r-" .

Will do, I just like criticizing myself :)
Comment 16 WebKit Commit Bot 2011-01-17 19:10:50 PST
Comment on attachment 79211 [details]
Proposed patch

Clearing flags on attachment: 79211

Committed r75992: <http://trac.webkit.org/changeset/75992>
Comment 17 WebKit Commit Bot 2011-01-17 19:10:56 PST
All reviewed patches have been landed.  Closing bug.