Bug 104613 - [WK2] Fix memory sampler for ARM Linux
Summary: [WK2] Fix memory sampler for ARM Linux
Status: RESOLVED DUPLICATE of bug 144439
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: JungJik Lee
URL:
Keywords:
Depends on:
Blocks: 108645
  Show dependency treegraph
 
Reported: 2012-12-10 16:51 PST by Laszlo Gombos
Modified: 2015-05-11 02:19 PDT (History)
6 users (show)

See Also:


Attachments
fix (1.77 KB, patch)
2012-12-10 17:12 PST, Laszlo Gombos
no flags Details | Formatted Diff | Diff
2nd try (1.77 KB, patch)
2012-12-10 19:15 PST, Laszlo Gombos
benjamin: review-
Details | Formatted Diff | Diff
use a explicit c++ type cast and use feof for checking file-end. (1.78 KB, patch)
2013-02-14 03:26 PST, JungJik Lee
no flags Details | Formatted Diff | Diff
Patch (1.99 KB, patch)
2013-02-18 00:13 PST, JungJik Lee
ossy: review-
ossy: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Laszlo Gombos 2012-12-10 16:51:21 PST
Building for ARM Linux gives the following error (e.g. on EFL port)

/webkit/Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp: In function ‘WTF::String WebKit::nextToken(FILE*)’:
/webkit/Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:69:19: error: comparison is always false due to limited range of data type

The type "char" by default is signed on x86 and unsigned on ARM. On platforms where "char" is unsigned the value -1 (EOF) becomes 255 when stored in an unsigned char (variable ch in the code above). This can easily lead to an infinite loop because the end of the file cannot be recognized.
Comment 1 Laszlo Gombos 2012-12-10 17:12:10 PST
Created attachment 178671 [details]
fix

Make sure that the return value for fgetc() is first assigned to a signed variable to check for EOF and avoid the potential for an infinite loop.
Comment 2 Laszlo Gombos 2012-12-10 19:15:06 PST
Created attachment 178689 [details]
2nd try

Swap the conditions for the while as it was in the original code.
Comment 3 Kenneth Rohde Christiansen 2012-12-10 23:54:34 PST
Comment on attachment 178689 [details]
2nd try

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

> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:-69
> -        if (ch == EOF || (isASCIISpace(ch) && index)) // Break on non-initial ASCII space.

I think you have the same issue in Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp
Comment 4 Laszlo Gombos 2012-12-11 06:59:20 PST
(In reply to comment #3)
> (From update of attachment 178689 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178689&action=review
> 
> > Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:-69
> > -        if (ch == EOF || (isASCIISpace(ch) && index)) // Break on non-initial ASCII space.
> 
> I think you have the same issue in Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp

NetscapePluginModuleX11.cpp looks good to me as the return value of fputc is assigned to a signed variable (int is signed).

int result;
while ((result = fputc(*current, stdout)) == EOF && errno == EINTR) { }
Comment 5 JungJik Lee 2012-12-11 20:50:27 PST
Comment on attachment 178689 [details]
2nd try

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

>>> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:-69
>>> -        if (ch == EOF || (isASCIISpace(ch) && index)) // Break on non-initial ASCII space.
>> 
>> I think you have the same issue in Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp
> 
> NetscapePluginModuleX11.cpp looks good to me as the return value of fputc is assigned to a signed variable (int is signed).
> 
> int result;
> while ((result = fputc(*current, stdout)) == EOF && errno == EINTR) { }

I didn't think enough. Could we use feof(file) instead of ch == EOF?
if (feof(file) || ferror(file) || (isASCIISpace(ch) && index))
Comment 6 Benjamin Poulain 2013-01-08 19:58:12 PST
Comment on attachment 178689 [details]
2nd try

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

> Source/WebKit2/ChangeLog:11
> +
> +        Make sure that the return value for fgetc() is first assigned to a
> +        signed variable to check for EOF and avoid the potential for an infinite
> +        loop. This is important for ARM platforms where "char" type is
> +        unsigned by default.

This whole signed-unsigned char is not the problem. The problem is the assignment from fgetc() was a one with loss. It is incorrect, ARM or not.

> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:70
> +    int ch;
> +    while ((index < maxBuffer) && (ch = fgetc(file) != EOF)) {
> +        char charCode = ch;
> +        if (isASCIISpace(charCode) && index) // Break on non-initial ASCII space.

This code is harder to read than it should.

Please split your condition and assignment to make something nice and clean.
You probably also want a c++ cast for int->char to make it explicit.
Comment 7 JungJik Lee 2013-02-14 03:26:55 PST
Created attachment 188299 [details]
use a explicit c++ type cast and use feof for checking file-end.
Comment 8 Chris Dumez 2013-02-17 00:29:35 PST
Comment on attachment 188299 [details]
use a explicit c++ type cast and use feof for checking file-end.

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

The change makes sense.

> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:70
> +        if ((isASCIISpace(charCode) && index) || feof(file) || ferror(file)) // Break on non-initial ASCII space.

I think it would be conceptually nicer to check for end of file and error *before* checking for space. I would reverse the conditions:
if (feof(file) || ferror(file) || (isASCIISpace(charCode) && index))
Comment 9 JungJik Lee 2013-02-18 00:13:41 PST
Created attachment 188806 [details]
Patch
Comment 10 JungJik Lee 2013-02-18 00:17:46 PST
(In reply to comment #8)
> (From update of attachment 188299 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188299&action=review
> 
> The change makes sense.
> 
> > Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:70
> > +        if ((isASCIISpace(charCode) && index) || feof(file) || ferror(file)) // Break on non-initial ASCII space.
> 
> I think it would be conceptually nicer to check for end of file and error *before* checking for space. I would reverse the conditions:
> if (feof(file) || ferror(file) || (isASCIISpace(charCode) && index))

Taking your advice, I filed new patch. thanks for your opinion.
Comment 11 Csaba Osztrogonác 2015-05-11 02:18:47 PDT

*** This bug has been marked as a duplicate of bug 144439 ***