Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1227383003:
Remove memset from VideoFrame and mark buffer as unpoisoned (Closed)
DescriptionRemove memset from VideoFrame and mark buffer as unpoisoned
- I removed the memset from AllocateYUV as it looks unnecessary except for FFmpegVideoDecoder.
- FFmpegVideoDecoder marks the incoming buffer from VideoFramePool as unpoisoned.
BUG=508220
Committed: https://crrev.com/3188c15f1928d96621aab55b575230284709a9c6
Cr-Commit-Position: refs/heads/master@{#339304}
Patch Set 1 #
Total comments: 7
Patch Set 2 : dalecurtis@ comments #
Total comments: 8
Patch Set 3 : dalecurtis@ comments. #
Total comments: 4
Patch Set 4 : Move tests and commments. #Patch Set 5 : Reverted av_buffer_create size change. #Patch Set 6 : Move unpoisoned inside ffmpeg_video_decoder. #
Messages
Total messages: 32 (5 generated)
emircan@chromium.org changed reviewers: + dalecurtis@chromium.org, mcasas@chromium.org, miu@chromium.org
Please take a look.
dalecurtis@chromium.org changed reviewers: + wfh@chromium.org
Note, this might also allow attackers to get at large chunks of uninitialized memory. Do we automatically zero allocations done in NaCl? +wfh from security for any considerations. https://codereview.chromium.org/1227383003/diff/1/media/filters/ffmpeg_video_... File media/filters/ffmpeg_video_decoder.cc (right): https://codereview.chromium.org/1227383003/diff/1/media/filters/ffmpeg_video_... media/filters/ffmpeg_video_decoder.cc:133: memset(video_frame->data(0), 0, data_size); We don't want to do this here actually, as now memset() is called for every video frame instead of for every new video frame. Instead we should have an option for the VideoFramePool to do this on initial allocation only.
https://codereview.chromium.org/1227383003/diff/1/media/filters/ffmpeg_video_... File media/filters/ffmpeg_video_decoder.cc (left): https://codereview.chromium.org/1227383003/diff/1/media/filters/ffmpeg_video_... media/filters/ffmpeg_video_decoder.cc:151: VideoFrame::AllocationSize(format, coded_size), Hmm, I think we were intentionally using the lower size here such that ffmpeg didn't think it had further padding areas to read/write from (which it does in many places).
(fyi make sure you send this manually to the msan/valgrind trybots)
Thanks dalecurtis@. It looks like all the users of CreateFrame() are overwriting it with new buffers and such, but if there are other security concerns, please let me know. https://codereview.chromium.org/1227383003/diff/1/media/filters/ffmpeg_video_... File media/filters/ffmpeg_video_decoder.cc (left): https://codereview.chromium.org/1227383003/diff/1/media/filters/ffmpeg_video_... media/filters/ffmpeg_video_decoder.cc:151: VideoFrame::AllocationSize(format, coded_size), On 2015/07/09 21:36:22, DaleCurtis wrote: > Hmm, I think we were intentionally using the lower size here such that ffmpeg > didn't think it had further padding areas to read/write from (which it does in > many places). I see. It looked like a mistake to me in the beginning as frame->data[] points to the aligned&padded buffers and av_buffer_create goes for the packed size, which is a mismatch. If we are not using such padded areas, should we remove them from allocation and only allocate the packed size instead? https://codereview.chromium.org/1227383003/diff/1/media/filters/ffmpeg_video_... File media/filters/ffmpeg_video_decoder.cc (right): https://codereview.chromium.org/1227383003/diff/1/media/filters/ffmpeg_video_... media/filters/ffmpeg_video_decoder.cc:133: memset(video_frame->data(0), 0, data_size); On 2015/07/09 21:35:11, DaleCurtis wrote: > We don't want to do this here actually, as now memset() is called for every > video frame instead of for every new video frame. Instead we should have an > option for the VideoFramePool to do this on initial allocation only. Done. I added |zero_out_memory| bool to carry that information to the creation.
https://codereview.chromium.org/1227383003/diff/1/media/filters/ffmpeg_video_... File media/filters/ffmpeg_video_decoder.cc (left): https://codereview.chromium.org/1227383003/diff/1/media/filters/ffmpeg_video_... media/filters/ffmpeg_video_decoder.cc:151: VideoFrame::AllocationSize(format, coded_size), On 2015/07/09 22:12:17, emircan wrote: > On 2015/07/09 21:36:22, DaleCurtis wrote: > > Hmm, I think we were intentionally using the lower size here such that ffmpeg > > didn't think it had further padding areas to read/write from (which it does in > > many places). > > I see. It looked like a mistake to me in the beginning as frame->data[] points > to the aligned&padded buffers and av_buffer_create goes for the packed size, > which is a mismatch. > If we are not using such padded areas, should we remove them from allocation and > only allocate the packed size instead? Hmm, I see, we can try it your way and see what falls out. https://codereview.chromium.org/1227383003/diff/20001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/1227383003/diff/20001/media/base/video_frame.... media/base/video_frame.cc:551: size_t VideoFrame::AlignedAllocationSize( This logic shouldn't be duplicated relative to the method below, or at the very least a DCHECK() should enforce the two. https://codereview.chromium.org/1227383003/diff/20001/media/base/video_frame_... File media/base/video_frame_pool.cc (right): https://codereview.chromium.org/1227383003/diff/20001/media/base/video_frame_... media/base/video_frame_pool.cc:28: bool zero_out_memory); This is a bit confusing since it would only happen on new frames and not reused frames, so we either need a better name or this should be a constructor argument with some more details. Either way a comment should be added explaining the parameter precisely. Maybe "zero_initialize_new_allocations" ? Though it is a bit wordy.
https://codereview.chromium.org/1227383003/diff/1/media/filters/ffmpeg_video_... File media/filters/ffmpeg_video_decoder.cc (left): https://codereview.chromium.org/1227383003/diff/1/media/filters/ffmpeg_video_... media/filters/ffmpeg_video_decoder.cc:151: VideoFrame::AllocationSize(format, coded_size), On 2015/07/09 22:21:16, DaleCurtis wrote: > On 2015/07/09 22:12:17, emircan wrote: > > On 2015/07/09 21:36:22, DaleCurtis wrote: > > > Hmm, I think we were intentionally using the lower size here such that > ffmpeg > > > didn't think it had further padding areas to read/write from (which it does > in > > > many places). > > > > I see. It looked like a mistake to me in the beginning as frame->data[] points > > to the aligned&padded buffers and av_buffer_create goes for the packed size, > > which is a mismatch. > > If we are not using such padded areas, should we remove them from allocation > and > > only allocate the packed size instead? > > Hmm, I see, we can try it your way and see what falls out. Done. https://codereview.chromium.org/1227383003/diff/20001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/1227383003/diff/20001/media/base/video_frame.... media/base/video_frame.cc:551: size_t VideoFrame::AlignedAllocationSize( On 2015/07/09 22:21:16, DaleCurtis wrote: > This logic shouldn't be duplicated relative to the method below, or at the very > least a DCHECK() should enforce the two. I added a DCHECK at the end of AllocateYUV() to enforce the match. I decided to keep it as a static function as the ongoing effort is to make VideoFrame class' footprint to be smaller. https://codereview.chromium.org/1227383003/diff/20001/media/base/video_frame_... File media/base/video_frame_pool.cc (right): https://codereview.chromium.org/1227383003/diff/20001/media/base/video_frame_... media/base/video_frame_pool.cc:28: bool zero_out_memory); On 2015/07/09 22:21:16, DaleCurtis wrote: > This is a bit confusing since it would only happen on new frames and not reused > frames, so we either need a better name or this should be a constructor argument > with some more details. Either way a comment should be added explaining the > parameter precisely. Maybe "zero_initialize_new_allocations" ? Though it is a > bit wordy. True, I will go with "zero_init_new_allocations".
I think with the additional comment context we can get away with |zero_new_frames| since the pool explains the concept of new vs reuse frames. https://codereview.chromium.org/1227383003/diff/40001/media/base/video_frame_... File media/base/video_frame_pool.cc (right): https://codereview.chromium.org/1227383003/diff/40001/media/base/video_frame_... media/base/video_frame_pool.cc:23: // If it is to create a new frame and |zero_init_new_allocations| is nit: If a new frame must be created and |zero_new_frames| is true, the buffer for the new frame will be zero initialized. Reused frames will not be zero initialized. Actually this comment should go in the video_frame_pool.h file, no need to duplicate it here. https://codereview.chromium.org/1227383003/diff/40001/media/base/video_frame_... File media/base/video_frame_unittest.cc (right): https://codereview.chromium.org/1227383003/diff/40001/media/base/video_frame_... media/base/video_frame_unittest.cc:334: TEST(VideoFrame, InitializedAndZeroed) { Just move to the frame pool unittests?
https://codereview.chromium.org/1227383003/diff/20001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1227383003/diff/20001/media/base/video_frame.... media/base/video_frame.h:291: static size_t AlignedAllocationSize(const scoped_refptr<VideoFrame>& frame); How is this any different from frame->coded_size().GetArea()? Seems like it computes the same thing. https://codereview.chromium.org/1227383003/diff/20001/media/filters/ffmpeg_vi... File media/filters/ffmpeg_video_decoder.cc (right): https://codereview.chromium.org/1227383003/diff/20001/media/filters/ffmpeg_vi... media/filters/ffmpeg_video_decoder.cc:129: // to do so can lead to unitialized value usage. See http://crbug.com/390941 Instead of ever initializing the memory, why not just mark the region as "unpoisoned" so tools like MSAN won't complain? See: https://code.google.com/p/chromium/codesearch#chromium/src/base/compiler_spec...
https://codereview.chromium.org/1227383003/diff/20001/media/filters/ffmpeg_vi... File media/filters/ffmpeg_video_decoder.cc (right): https://codereview.chromium.org/1227383003/diff/20001/media/filters/ffmpeg_vi... media/filters/ffmpeg_video_decoder.cc:129: // to do so can lead to unitialized value usage. See http://crbug.com/390941 On 2015/07/09 23:31:34, miu wrote: > Instead of ever initializing the memory, why not just mark the region as > "unpoisoned" so tools like MSAN won't complain? > > See: > https://code.google.com/p/chromium/codesearch#chromium/src/base/compiler_spec... If we're okay with potentially large allocations being readable via canvas, that's fine with me. Hopefully wfh@ has some commentary :) If we don't zero initialize nacl allocations today, it's a moot point.
On 2015/07/09 23:34:43, DaleCurtis wrote: > https://codereview.chromium.org/1227383003/diff/20001/media/filters/ffmpeg_vi... > File media/filters/ffmpeg_video_decoder.cc (right): > > https://codereview.chromium.org/1227383003/diff/20001/media/filters/ffmpeg_vi... > media/filters/ffmpeg_video_decoder.cc:129: // to do so can lead to unitialized > value usage. See http://crbug.com/390941 > On 2015/07/09 23:31:34, miu wrote: > > Instead of ever initializing the memory, why not just mark the region as > > "unpoisoned" so tools like MSAN won't complain? > > > > See: > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/compiler_spec... > > If we're okay with potentially large allocations being readable via canvas, > that's fine with me. Hopefully wfh@ has some commentary :) If we don't zero > initialize nacl allocations today, it's a moot point. these allocations are happening inside the renderer, or are renderers able to control allocations in other processes? (sorry, I'm trying to follow the CL but the code is unfamiliar to me)
On 2015/07/09 23:40:06, Will Harris wrote: > On 2015/07/09 23:34:43, DaleCurtis wrote: > > > https://codereview.chromium.org/1227383003/diff/20001/media/filters/ffmpeg_vi... > > File media/filters/ffmpeg_video_decoder.cc (right): > > > > > https://codereview.chromium.org/1227383003/diff/20001/media/filters/ffmpeg_vi... > > media/filters/ffmpeg_video_decoder.cc:129: // to do so can lead to unitialized > > value usage. See http://crbug.com/390941 > > On 2015/07/09 23:31:34, miu wrote: > > > Instead of ever initializing the memory, why not just mark the region as > > > "unpoisoned" so tools like MSAN won't complain? > > > > > > See: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/compiler_spec... > > > > If we're okay with potentially large allocations being readable via canvas, > > that's fine with me. Hopefully wfh@ has some commentary :) If we don't zero > > initialize nacl allocations today, it's a moot point. > > these allocations are happening inside the renderer, or are renderers able to > control allocations in other processes? (sorry, I'm trying to follow the CL but > the code is unfamiliar to me) The case under discussion is in the renderer process. I think there are frames allocated in renderer and gpu for sure, maybe the browser process too.
CreateFrame has many users, and my motive in making a change was after realizing the cost of memset in the cases when VideoFramePool isn't used. For instance, in cases like cast[0], buffers will be zeroed out just to be overwritten right after. [0] https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/receive... I understand the security concerns, but I will leave it to your discussion. In the current patch, only FFmpegVideoDecoder will receive zero initialized buffers in the beginning. https://codereview.chromium.org/1227383003/diff/20001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1227383003/diff/20001/media/base/video_frame.... media/base/video_frame.h:291: static size_t AlignedAllocationSize(const scoped_refptr<VideoFrame>& frame); On 2015/07/09 23:31:34, miu wrote: > How is this any different from frame->coded_size().GetArea()? Seems like it > computes the same thing. VideoFrame::AllocateYUV() is using constants kFrameSizePadding and kFrameSizeAlignment, and ends up being greater than that. https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_f... https://codereview.chromium.org/1227383003/diff/40001/media/base/video_frame_... File media/base/video_frame_pool.cc (right): https://codereview.chromium.org/1227383003/diff/40001/media/base/video_frame_... media/base/video_frame_pool.cc:23: // If it is to create a new frame and |zero_init_new_allocations| is On 2015/07/09 23:29:44, DaleCurtis wrote: > nit: If a new frame must be created and |zero_new_frames| is true, the buffer > for the new frame will be zero initialized. Reused frames will not be zero > initialized. > > Actually this comment should go in the video_frame_pool.h file, no need to > duplicate it here. Done. https://codereview.chromium.org/1227383003/diff/40001/media/base/video_frame_... File media/base/video_frame_unittest.cc (right): https://codereview.chromium.org/1227383003/diff/40001/media/base/video_frame_... media/base/video_frame_unittest.cc:334: TEST(VideoFrame, InitializedAndZeroed) { On 2015/07/09 23:29:44, DaleCurtis wrote: > Just move to the frame pool unittests? Done.
lgtm, but defer to wfh@ if a annotation based approach can work instead.
lots of red on the trybots, is there another PS coming?
I still don't understand why initialising the memory is necessary at all. What are the security concerns here?
On 2015/07/10 17:43:24, Will Harris wrote: > lots of red on the trybots, is there another PS coming? I just reverted the change below, and bots look better now. https://codereview.chromium.org/1227383003/diff/1/media/filters/ffmpeg_video_... File media/filters/ffmpeg_video_decoder.cc (left): https://codereview.chromium.org/1227383003/diff/1/media/filters/ffmpeg_video_... media/filters/ffmpeg_video_decoder.cc:151: VideoFrame::AllocationSize(format, coded_size), On 2015/07/09 23:16:27, emircan wrote: > On 2015/07/09 22:21:16, DaleCurtis wrote: > > On 2015/07/09 22:12:17, emircan wrote: > > > On 2015/07/09 21:36:22, DaleCurtis wrote: > > > > Hmm, I think we were intentionally using the lower size here such that > > ffmpeg > > > > didn't think it had further padding areas to read/write from (which it > does > > in > > > > many places). > > > > > > I see. It looked like a mistake to me in the beginning as frame->data[] > points > > > to the aligned&padded buffers and av_buffer_create goes for the packed size, > > > which is a mismatch. > > > If we are not using such padded areas, should we remove them from allocation > > and > > > only allocate the packed size instead? > > > > Hmm, I see, we can try it your way and see what falls out. > > Done. Well, bots showed that it didn't work out. Reverting this.
On 2015/07/10 17:56:41, miu wrote: > I still don't understand why initialising the memory is necessary at all. What > are the security concerns here? My concern is that large uninitialized blocks of memory are more likely to contain sensitive information. Though I'd forgotten we're no longer lenient to video decode errors, so my concern is a lot less now. Unless someone can generate video frames that are not errors to ffmpeg and still include uninitialized information, it shouldn't be a problem to just mark the region as unpoisoned. emerican@ do you want to take a stab at that change?
Patchset #6 (id:100001) has been deleted
Patchset #6 (id:120001) has been deleted
On 2015/07/13 18:23:34, DaleCurtis wrote: > My concern is that large uninitialized blocks of memory are more likely to > contain sensitive information. Though I'd forgotten we're no longer lenient to > video decode errors, so my concern is a lot less now. Unless someone can > generate video frames that are not errors to ffmpeg and still include > uninitialized information, it shouldn't be a problem to just mark the region as > unpoisoned. emerican@ do you want to take a stab at that change? I moved the unpoisoned mark inside ffmpeg each time frame is received. What do you think?
lgtm, but I'd run valgrind locally and send it against the msan bot.
can you update the CL description please?
lgtm
Message was sent while issue was closed.
Thanks. I updated the CL description. I ran Valgrind on my local machine and didn't come across to an error related to these classes. Asan bots look very flaky at this point. I will commit and keep an eye on possible errors reported.
The CQ bit was checked by emircan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1227383003/140001
asan bots likely won't be a problem, the msan bot (and clusterfuzz itself) is the one that is likely to complain.
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3188c15f1928d96621aab55b575230284709a9c6 Cr-Commit-Position: refs/heads/master@{#339304} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
