I was using examples/simple_encoder.c from libvpx to find this issue. The command line I used was: $./simple_encoder vp9 64 36 test_case.yuv /dev/null 10 ==18848==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000004 (pc 0x00000082ccd0 bp 0x7ffe54d2e5b0 sp 0x7ffe54d2e5a0 T0) #0 0x82cccf in vp9_lookahead_pop (/home/user/Desktop/vpx/simple_encoder+0x82cccf) #1 0x5b0841 in vp9_get_compressed_data (/home/user/Desktop/vpx/simple_encoder+0x5b0841) #2 0x58cee3 in encoder_encode (/home/user/Desktop/vpx/simple_encoder+0x58cee3) #3 0x4fd4d4 in vpx_codec_encode (/home/user/Desktop/vpx/simple_encoder+0x4fd4d4) #4 0x4fbada in encode_frame /home/user/code/libvpx/examples/simple_encoder.c:126:31 #5 0x4fb4a3 in main /home/user/code/libvpx/examples/simple_encoder.c:246:10 #6 0x7f5b1b5b6ec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287 #7 0x42ecb5 in _start (/home/user/Desktop/vpx/simple_encoder+0x42ecb5) AddressSanitizer can not provide additional info. SUMMARY: AddressSanitizer: SEGV (/home/user/Desktop/vpx/simple_encoder+0x82cccf) in vp9_lookahead_pop ==18848==ABORTING Using WebM Project VP9 Encoder v1.5.0-293-g2bd4f44

Can you take a look, Gerald?

vpx_codec_encode may be called with its 2nd parameter 'img' set to NULL, to force it to encode any held buffers from previous calls. http://mxr.mozilla.org/mozilla-central/source/media/libvpx/vpx/vpx_encoder.h#920 In this case, there are no images that can be read in test_case.yuv, so the simple_encoder program goes straight to this img=NULL call, without ever calling it with actual images before that. Looking at libvpx code, it relies on having a first image to allocate appropriate data structures, including a 'lookahead_ctx'. So when this code first runs with no images, it assumes that lookahead_ctx exists and dereferences a pointer to it, causing this crash. Assuming the documentation is correct (and not missing a mention that vpx_codec_encode(img=NULL) should only be called *if* it was previously called with a non-NULL img), and because existing code (in libvpx's simple_encoder and in Firefox) does not check for this case, I will propose a fix that takes care of this situation in vpx_codec_encode itself.

Actually, this may apply to all VPx versions, and we currently don't have VP10 in our tree, so a fix here would probably not be enough to port upstream. Also, I don't believe this is a security bug (null-dereference), or even a commonplace issue (stopping encoding after 0 images), so I think we could just let libvpx get fixed upstream, and we'll eventually get it in our tree. James: Here's my proposed fix for VP8 and VP9: https://hg.mozilla.org/try/rev/ab86819aa6b4 (You may have to produce a similar fix in VP10.) - Does it look correct to you? - Do you agree with my assessment that it is low-risk? - Could you please patch libvpx? (with it or a different/better fix, as you see fit, of course)

Thanks for the detail in the report. At a quick glance this seems to be vp9/10-only since the allocation was deferred to receipt of the first frame in vp9 then the behavior copied over to vp10. I'll take a closer look and post a patch when I get a chance.

This change [1] should fix the issue. I realized that I had this basic encoder api test (based on an earlier decode api test) locally with this line commented out since last year sometime. It's unfortunate it took so long to get minimal coverage here. [1] https://chromium-review.googlesource.com/324510

That change has been submitted. Thanks again for the report.

Copied from https://chromium-review.googlesource.com/324510 Review commit: https://reviewboard.mozilla.org/r/33355/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/33355/

Review commit: https://reviewboard.mozilla.org/r/33357/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/33357/

Comment on attachment 8715136 [details] MozReview Request: Bug 1237848 - Check lookahead ctx - r?rillian Mozreview seems to be down still. r=me in bugzilla.

Comment on attachment 8715136 [details] MozReview Request: Bug 1237848 - Check lookahead ctx - r?rillian https://reviewboard.mozilla.org/r/33355/#review30205 (see bug 1237848 comment 9)

(In reply to Ralph Giles (:rillian) from comment #9) > Mozreview seems to be down still. r=me in bugzilla. Doesn't seem "down" to me (I can access the website and tried to "ship it"), though that publish action didn't seem to go all the way -- I'm guessing that's what you mean. Anyway, thank you for the review, I'll land the patches soon.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bee117e3835 https://hg.mozilla.org/integration/mozilla-inbound/rev/f6e4f2f869a3

https://hg.mozilla.org/mozilla-central/rev/3bee117e3835 https://hg.mozilla.org/mozilla-central/rev/f6e4f2f869a3

Gerald, should we uplift this fix to Beta45 and Aurora46?

Comment on attachment 8715136 [details] MozReview Request: Bug 1237848 - Check lookahead ctx - r?rillian Approval Request Comment [Feature/regressing bug #]: MP4 playback and recording [User impact if declined]: Potential crash if media has zero frames (very unlikely in real life) [Describe test coverage new/current, TreeHerder]: Lots of media tests, I'll start aurora&beta try runs shortly but I'm confident they'll be fine for this change. [Risks and why]: This patch is only adding null-checks before using pointers (very low risk), and an assert to prove that another pointer cannot be null (low risk, has never triggered since landed). [String/UUID change made/needed]: None.

Note: Only the first patch should be uplifted (if approved), *not* the other one.

Aurora try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b1a12a9490c Beta try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc3c1123adc4

Comment on attachment 8715136 [details] MozReview Request: Bug 1237848 - Check lookahead ctx - r?rillian Wrote "MP4" playback in initial request, it should be VP9. (Thank you :jya for noticing) Approval Request Comment [Feature/regressing bug #]: VP9 playback and recording [User impact if declined]: Potential crash if media has zero frames (very unlikely in real life) [Describe test coverage new/current, TreeHerder]: Lots of media tests, aurora&beta try runs. [Risks and why]: This patch is only adding null-checks before using pointers in VP9 (very low risk), and an assert to prove that another pointer cannot be null in VP8 (low risk, has never triggered since landed). [String/UUID change made/needed]: None.

Comment on attachment 8715136 [details] MozReview Request: Bug 1237848 - Check lookahead ctx - r?rillian Fix a crash, I want it! Should be in 45 beta 9

Comment on attachment 8715137 [details] MozReview Request: Bug 1237848 - Updated update.py patch - r?rillian Explicit comment #16

https://reviewboard.mozilla.org/r/33357/#review30201 This is r+ on the bug. Not sure why it shows unreviewed here.