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-
Proposed patch (1.66 KB, patch)
2011-01-17 13:13 PST, Jarred Nicholls
darin: review-
darin: commit-queue-
Proposed patch (1.50 KB, patch)
2011-01-17 13:24 PST, Jarred Nicholls
jarred: review-
Proposed patch (1.50 KB, patch)
2011-01-17 13:26 PST, Jarred Nicholls
no flags
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.