Bug 125674 – Fix unsafe memory load/store from the argument encoder/decoder affecting ARM

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

[ARM] Fix unsafe memory load/store from the argument encoder/decoder

[Benjamin Poulain]

Created attachment 219147 [details]
Patch

[Darin Adler]

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

[Darin Adler]

Since the code change is in platform-independent code, the bug title should not have a platform prefix.

[Benjamin Poulain]

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

[Benjamin Poulain]

Committed r160529: <http://trac.webkit.org/changeset/160529>

[Benjamin Poulain]

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.

[Darin Adler]

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