Summary: | [WK2] Fix memory sampler for ARM Linux | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Laszlo Gombos <laszlo.gombos> | ||||||||||
Component: | WebKit2 | Assignee: | JungJik Lee <jungjik.lee> | ||||||||||
Status: | RESOLVED DUPLICATE | ||||||||||||
Severity: | Normal | CC: | benjamin, cdumez, hausmann, jungjik.lee, kenneth, ossy | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 108645 | ||||||||||||
Attachments: |
|
Description
Laszlo Gombos
2012-12-10 16:51:21 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.
Created attachment 178689 [details]
2nd try
Swap the conditions for the while as it was in the original code.
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 (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 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 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. Created attachment 188299 [details]
use a explicit c++ type cast and use feof for checking file-end.
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)) Created attachment 188806 [details]
Patch
(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. *** This bug has been marked as a duplicate of bug 144439 *** |