Bug 104613 – [WK2] Fix memory sampler for ARM Linux

• Component: WebKit2
• Status: RESOLVED DUPLICATE

[Laszlo Gombos]

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.

[Laszlo Gombos]

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.

[Laszlo Gombos]

Created attachment 178689 [details]
2nd try

Swap the conditions for the while as it was in the original code.

[Kenneth Rohde Christiansen]

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

[Laszlo Gombos]

(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) { }

[JungJik Lee]

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))

[Benjamin Poulain]

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.

[JungJik Lee]

Created attachment 188299 [details]
use a explicit c++ type cast and use feof for checking file-end.

[Chris Dumez]

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))

[JungJik Lee]

Created attachment 188806 [details]
Patch

[JungJik Lee]

(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.

[Csaba Osztrogonác]

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