[FFmpeg-devel] [PATCH 2/3] avformat: reject FFmpeg-style merged side data in raw packets

[wm4]

On Wed, 8 Mar 2017 20:42:17 +0100
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Wed, Mar 08, 2017 at 08:23:26PM +0100, wm4 wrote:
> > On Wed, 8 Mar 2017 20:15:24 +0100
> > Michael Niedermayer <michael at niedermayer.cc> wrote:
> >   
> > > On Wed, Mar 08, 2017 at 07:35:50PM +0100, wm4 wrote:  
> > > > On Wed, 8 Mar 2017 19:20:15 +0100
> > > > Michael Niedermayer <michael at niedermayer.cc> wrote:
> > > >     
> > > > > On Wed, Mar 08, 2017 at 05:26:57PM +0100, wm4 wrote:    
> > > > > > On Wed, 8 Mar 2017 17:11:12 +0100
> > > > > > Michael Niedermayer <michael at niedermayer.cc> wrote:
> > > > > >       
> > > > > > > On Wed, Mar 08, 2017 at 04:06:20PM +0100, wm4 wrote:      
> > > > > > > > On Wed, 8 Mar 2017 15:36:25 +0100
> > > > > > > > Michael Niedermayer <michael at niedermayer.cc> wrote:
> > > > > > > >         
> > > > > > > > > On Wed, Mar 08, 2017 at 01:40:11PM +0100, wm4 wrote:        
> > > > > [...]    
> > > > > > >      
> > > > > > > >         
> > > > > > > > > also it may be interresting to disable this check for fuzzing so
> > > > > > > > > side data can be fuzzed in a wider range of cases and any past
> > > > > > > > > testcases that happen to use this can still be used for regression
> > > > > > > > > testing        
> > > > > > > > 
> > > > > > > > I think what you want is fault injection for memory errors, seems out
> > > > > > > > of scope here.        
> > > > > > > 
> > > > > > > no, i want fuzzing to continue to fuzz side data, it did so in the
> > > > > > > past and it should continue to do so.      
> > > > > > 
> > > > > > You can fuzz side data as much as you can fuzz AVFrame or
> > > > > > AVCodecContext. I believe randomly changing in-memory data structures
> > > > > > is referred to as fault injection, not fuzzing.      
> > > > > 
> > > > > it doesnt really matter what you call it, but it was done and the
> > > > > patch breaks it if theres no option to disable it or something else    
> > > > 
> > > > PS: honestly, I think you're trolling with this. The possibility that
> > > > random packets could be interpreted as side-data doesn't look like
> > > > something that was accounted for, and I highly doubt this plays a role
> > > > for fuzzing (or ever occurred in a fuzzing case). Why are you so
> > > > hell-bent in finally preventing this arguably dangerous corner case? It
> > > > makes no sense at all. Explain yourself.    
> > > 
> > > It plays a role, for example the timeout in 731 goes away if
> > > i change FF_MERGE_MARKER and the value prior to teh change is in the
> > > sample according to my hex editor
> > > 
> > > so the fuzzer seems capable to create data to get past the check
> > > iam not trolling and this does play a role, ive seen other issues
> > > that used the side data split
> > > 
> > > how the fuzzer does this i dont know but i can imagine a few ways
> > > like looking at the individual bytes of the comparission and evolving
> > > towards breaking it or maybe some input file was generated by
> > > libavformat leaking the marker into it somehow.  
> > 
> > If you argue this way, the fuzzer will expose issues that can happen
> > with the normal data flow too. (For example, if a demuxer sets a field
> > in a side data struct to a value read from the file - obviously the
> > fuzzer will be capable of exposing bugs related in readers of this
> > field.)  
> 
> > 
> > Normally you would be glad that an issue the fuzzer previously exposed
> > goes away completely.  
> 
> It doesnt go completely away, it just closes an easy way to introduce
> side data

Well, in the first place it prevents accidental interpretation of input
data as merged side data. You seem to be sad about the fact that the
fuzzer won't be able to test such accidentally interpreted side data,
which makes no sense to me.

> but yes, iam in fact happy if the path marker->libavformat->libavcodec
> goes away

Like what?

> its the details iam not completely happy with

Which is? Other than the ENOMEM issue (will fix), strcmp (you didn't
reply to my remarks), and the idea to introduce a special function to
check for merged side data (I'm hesitant, maybe will do).