Summary: | Fix unsafe memory load/store from the argument encoder/decoder affecting ARM | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Benjamin Poulain <benjamin> | ||||
Component: | New Bugs | Assignee: | Benjamin Poulain <benjamin> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | darin, ossy | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 108645 | ||||||
Attachments: |
|
Description
Benjamin Poulain
2013-12-12 20:23:40 PST
Created attachment 219147 [details]
Patch
Comment on attachment 219147 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=219147&action=review > Source/WebKit2/Platform/CoreIPC/ArgumentDecoder.cpp:130 > +static void decodeValueFromBuffer(Type& value, uint8_t*& buffer) I don’t think that a pointer that we advance should be called “buffer”, because I think the word buffer refers to the entire buffer, not to a pointer within it. That’s why I think Anders was using the name “buffer position”. I would suggest naming the argument either bufferPosition or position. > Source/WebKit2/Platform/CoreIPC/ArgumentEncoder.cpp:135 > uint8_t* buffer = grow(sizeof(n), sizeof(n)); > - > - *reinterpret_cast<bool*>(buffer) = n; > + copyValueToBuffer(n, buffer); I would write these without the local variable: copyValueToBuffer(n, grow(sizeof(n), sizeof(n)); Since the code change is in platform-independent code, the bug title should not have a platform prefix. (In reply to comment #3) > Since the code change is in platform-independent code, the bug title should not have a platform prefix. Oh right, my bad. Committed r160529: <http://trac.webkit.org/changeset/160529> Thanks for the review!
> I would write these without the local variable:
>
> copyValueToBuffer(n, grow(sizeof(n), sizeof(n));
I am always on the edge in those cases.
In my opinion nested function calls sometime make the code harder to follow. I am also not a big fan of them due to the undefined order of evaluation between compilers/platforms.
In this case, the two sizeof() already add a lot of noise. I prefer to leave it on two lines.
(In reply to comment #6) > In my opinion nested function calls sometime make the code harder to follow. I agree. > I am also not a big fan of them due to the undefined order of evaluation between compilers/platforms. Yes, I avoid them when the functions have side effects for that reason. > In this case, the two sizeof() already add a lot of noise. Yes, I think I’d fix that with an overload of the grow function. > I prefer to leave it on two lines. OK with me. |