WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
52585
REGRESSION(
r75709
): Return value of fscanf() shouldn't be ignored.
https://bugs.webkit.org/show_bug.cgi?id=52585
Summary
REGRESSION(r75709): Return value of fscanf() shouldn't be ignored.
Jarred Nicholls
Reported
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.
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Jarred Nicholls
Comment 1
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.
Csaba Osztrogonác
Comment 2
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.
Csaba Osztrogonác
Comment 3
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.
Jarred Nicholls
Comment 4
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.
Csaba Osztrogonác
Comment 5
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)) {}
Csaba Osztrogonác
Comment 6
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? :)
Jarred Nicholls
Comment 7
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.
Csaba Osztrogonác
Comment 8
2011-01-17 13:14:48 PST
Comment on
attachment 79209
[details]
Proposed patch LGTM, r=me.
Darin Adler
Comment 9
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.
Jarred Nicholls
Comment 10
2011-01-17 13:19:54 PST
Spot on Darin, will revise.
Jarred Nicholls
Comment 11
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.
Jarred Nicholls
Comment 12
2011-01-17 13:26:28 PST
Created
attachment 79211
[details]
Proposed patch broken patch
Csaba Osztrogonác
Comment 13
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.
Csaba Osztrogonác
Comment 14
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-" .
Jarred Nicholls
Comment 15
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 :)
WebKit Commit Bot
Comment 16
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
>
WebKit Commit Bot
Comment 17
2011-01-17 19:10:56 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug