[vlc-devel] [vlc-commits] stream: assert that read size is well defined

[Rémi Denis-Courmont]

Le maanantaina 31. lokakuuta 2016, 9.04.51 EET Rémi Denis-Courmont a écrit :
> vlc | branch: master | Rémi Denis-Courmont <remi at remlab.net> | Mon Oct 31
> 10:00:39 2016 +0200| [3bf8a8ec60e5f97cbfbbe11e7f8c88ecb7935a45] |
> committer: Rémi Denis-Courmont
> 
> stream: assert that read size is well defined
> 
> The result of a read operation is a signed size_t, and cannot be
> negative (except on error). Thus reading more than SSIZE_MAX bytes at
> once is not well defined.
> 
> (Note: POSIX marks it as implementation-defined, and we cannot rely on
>  much given the different implementations.)
> 
> In practice, this is not really a limitation for regular reads as
> allocating a contiguous output buffer of more than SSIZE_MAX bytes is
> essentially impossible. It can however be a problem when skipping data
> (buffer pointer is NULL), especially on 32-bits platforms.
> 
> To skip such large amount of data, seeking is recommended instead,
> e.g.:
> 
>     vlc_stream_Seek(s, vlc_stream_Tell() + skip);
> 
> instead of:
> 
>     vlc_stream_Read(s, NULL, skip);

And yes, I know that the earlier will not work with nonseekable streams. But 
who in their right mind would want to read 2 GiB of data just to discard it? 
In other words, I would argue that if a demuxer needs to skip 2 GiB, the 
demuxer really only works with seekable streams.

With that said, there is still one unanswered question:

Should vlc_stream_Read(s, NULL, SSIZE_MAX+1) be undefined?
Or should it be defined to always fail like vlc_stream_Block(s, SSIZE_MAX+1)?

Keep in mind that it cannot be defined to transparently seek, as per the 
commit message, so do not suggest that as a third option.

Also note that changing ssize_t to uint64_t, uintmax_t or whatever larger 
integer type will also not work. A 32-bits computation will still overflow to 
a 32-bits result *before* that resulting value is passed to vlc_stream_Read() 
and up-converted to 64-bits.

-- 
Rémi Denis-Courmont
http://www.remlab.net/