[ARM] Fix unsafe memory load/store from the argument encoder/decoder
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.