Bug 125674

Summary: Fix unsafe memory load/store from the argument encoder/decoder affecting ARM
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: New BugsAssignee: 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 Flags
Patch darin: review+

Description Benjamin Poulain 2013-12-12 20:23:40 PST
[ARM] Fix unsafe memory load/store from the argument encoder/decoder
Comment 1 Benjamin Poulain 2013-12-12 20:43:52 PST
Created attachment 219147 [details]
Patch
Comment 2 Darin Adler 2013-12-12 20:55:31 PST
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));
Comment 3 Darin Adler 2013-12-12 20:55:54 PST
Since the code change is in platform-independent code, the bug title should not have a platform prefix.
Comment 4 Benjamin Poulain 2013-12-12 22:55:15 PST
(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.
Comment 5 Benjamin Poulain 2013-12-12 23:01:53 PST
Committed r160529: <http://trac.webkit.org/changeset/160529>
Comment 6 Benjamin Poulain 2013-12-12 23:04:26 PST
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.
Comment 7 Darin Adler 2013-12-13 07:56:24 PST
(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.