[FFmpeg-devel] fate: Do not report side data size

[wm4]

On Wed, 8 Mar 2017 14:09:53 +0100
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Wed, Mar 08, 2017 at 01:12:04PM +0100, wm4 wrote:
> > On Wed, 8 Mar 2017 13:04:11 +0100
> > Michael Niedermayer <michael at niedermayer.cc> wrote:
> >   
> > > On Wed, Mar 08, 2017 at 12:56:31PM +0100, wm4 wrote:  
> > > > On Wed, 8 Mar 2017 12:51:06 +0100
> > > > Michael Niedermayer <michael at niedermayer.cc> wrote:
> > > >     
> > > > > On Wed, Mar 08, 2017 at 12:28:17PM +0100, Hendrik Leppkes wrote:    
> > > > > > On Wed, Mar 8, 2017 at 1:17 AM, Vittorio Giovara
> > > > > > <vittorio.giovara at gmail.com> wrote:      
> > > > > > > This should address the mismatch between different archs      
> > > > > 
> > > > > iam not in favor of this solution
> > > > > 
> > > > >     
> > > > > > 
> > > > > > Removing the side_data_size from output should be fine, as its a
> > > > > > implementation detail and as seen here can even vary between
> > > > > > architecture or possibly even compiler.
> > > > > > Maybe someone that uses that ffprobe output more often can comment?      
> > > > > 
> > > > > I use all kinds of stuff
> > > > > if something is removed from ffprobes output it wont be tested anymore.
> > > > > We should test more not less.
> > > > > 
> > > > >     
> > > > > > 
> > > > > > An alternative for fixing fate would be to use fixed size fields in
> > > > > > the new sidedata, although the possibility of it breaking similarly
> > > > > > again in the future then remains.      
> > > > > 
> > > > > I strongly prefer fixed size to be used in side data over platform
> > > > > dependant fields. Not only does size become testable but theres also
> > > > > a platform specific difference less in the interface which should
> > > > > help bug reproducability between platforms
> > > > > 
> > > > > thanks
> > > > > 
> > > > > [...]    
> > > >     
> > >   
> > > > So why don't let we fate test e.g. sizeof(AVPacket)? Makes as much
> > > > sense.    
> > > 
> > > this is trolling  
> > 
> > Really, I'm not sure if your first post in this thread isn't trolling.
> >   
> > > No change in our code could have sizeof(AVPacket) be "wrong"
> > > but a
> > > int side_data_size
> > > can be wrong easily  
> > 
> > Side datas are generally structs (well, some aren't, but most newer
> > ones). How does it make sense to "test" the sizes of those structs?  
> 
> the "int side_data_size" matches the stuct sizeof only if it was
> set correctly. This field can be read from the main data of a AVPacket
> and in that case can be anything its also possible a simple typo in
> the code could set it incorrectly or linking to a old lib with a
> struct with fewer fields could cause it to be incorrect.

This could happen with anything. Except when a side-data-"merged"
packet was read from a raw file, which I consider a potentially
security relevant issue. I've sent a patch against it.

But in all other cases, do you have any reason to believe that any code
could accidentally set the wrong size in a way that wouldn't just be
normal C undefined behavior (like passing the wrong size to av_malloc)?

> 
> Theres alot that can make the field wrong.

Like what.

> displaying the size in the output and thus testing it is easy and
> usefull IMHO

Well, you still can do that, just not to FATE.

> 
> > 
> > If you want to do it right, then dump the side data contents with
> > ffprobe or whatever.  
> 
> Thats already done for some side data, but peple do not update the code
> when new cases are added. Also until now the size was platform
> independant in practice.

Then just make people add appropriate tests when side data is added.
New codecs, (de)muxers, and filters are also not tested just because
they are added, and need explicit fate tests. Why should it be
different here?

Currently, the side data is (probably) the same on all platforms only
by coincidence. Some structs use types that have no guaranteed fixed
size in the strict portable sense, but they're happen to be the same on
all platforms.

I don't think it's a good idea to rely on ABI coincidences. It's
literally the same as doing file I/O by writing entire structs with
single write/read calls.

In this case, size_t breaks the thing, but that's because size_t is
increasingly going to be used for memory sizes and (in this case) image
dimensions. (Although I find it stupid to use size_t for image
dimensions, but that's a different topic.)

> 
> If the size printing is removed then other code should be added
> to test for the size to match the correct value

Then it would be more reasonable to make av_packet_add_side_data()
check whether the size is correct for the given side data type.