| Message ID | 20191214221926.16074-2-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
On Sat, Dec 14, 2019 at 11:19:14PM +0100, Andreas Rheinhardt wrote: > bytestream2_get_bytes_left returns an unsigned int; as a result, > it returns big positive numbers if an overread already happened, > making it unsuitable for scenarios where one wants to allow this > in a controlled way (because the buffer is actually padded so that > no segfaults can happen). So change it to return an ordinary int. > > Also, bytestream2_get_bytes_left_p has been modified in the same way. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > Implemented James' suggestion to return an int. I have not found a place > where this would pose a problem. How do we know that this doesnt break any code ? [...]
On Sun, Dec 15, 2019 at 11:53 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > On Sat, Dec 14, 2019 at 11:19:14PM +0100, Andreas Rheinhardt wrote: > > bytestream2_get_bytes_left returns an unsigned int; as a result, > > it returns big positive numbers if an overread already happened, > > making it unsuitable for scenarios where one wants to allow this > > in a controlled way (because the buffer is actually padded so that > > no segfaults can happen). So change it to return an ordinary int. > > > > Also, bytestream2_get_bytes_left_p has been modified in the same way. > > > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > > --- > > Implemented James' suggestion to return an int. I have not found a place > > where this would pose a problem. > > How do we know that this doesnt break any code ? > > Well, first of all, the init functions take ints and assert that they are >= 0, so the ordinary return value of bytestream2_get_bytes_left[_p] is representable in an int. Second, I ran fate with asan and have only found a memleak in the hls muxer (will send a patch soon), but no problem with this patch. And finally, I have not found a problem in the places where these functions are used that I looked at. If you want all to be checked, then say so. I don't know how long this would take, though. - Andreas
On Mon, Dec 16, 2019 at 12:05:52AM +0100, Andreas Rheinhardt wrote: > On Sun, Dec 15, 2019 at 11:53 PM Michael Niedermayer <michael@niedermayer.cc> > wrote: > > > On Sat, Dec 14, 2019 at 11:19:14PM +0100, Andreas Rheinhardt wrote: > > > bytestream2_get_bytes_left returns an unsigned int; as a result, > > > it returns big positive numbers if an overread already happened, > > > making it unsuitable for scenarios where one wants to allow this > > > in a controlled way (because the buffer is actually padded so that > > > no segfaults can happen). So change it to return an ordinary int. > > > > > > Also, bytestream2_get_bytes_left_p has been modified in the same way. > > > > > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > > > --- > > > Implemented James' suggestion to return an int. I have not found a place > > > where this would pose a problem. > > > > How do we know that this doesnt break any code ? > > > > Well, first of all, the init functions take ints and assert that they are > >= 0, so the ordinary return value of bytestream2_get_bytes_left[_p] is > representable in an int. Second, I ran fate with asan and have only found a > memleak in the hls muxer (will send a patch soon), but no problem with this > patch. And finally, I have not found a problem in the places where these > functions are used that I looked at. If you want all to be checked, then > say so. I don't know how long this would take, though. what is the oppinion of other devs ? this is likely ok but some issue could be hiding in one of the many uses of this. Checking all would be a lot of work with little expected return Thanks [...]
diff --git a/libavcodec/bytestream.h b/libavcodec/bytestream.h index 7be7fc22fc..0516a6e3dc 100644 --- a/libavcodec/bytestream.h +++ b/libavcodec/bytestream.h @@ -151,12 +151,12 @@ static av_always_inline void bytestream2_init_writer(PutByteContext *p, p->eof = 0; } -static av_always_inline unsigned int bytestream2_get_bytes_left(GetByteContext *g) +static av_always_inline int bytestream2_get_bytes_left(GetByteContext *g) { return g->buffer_end - g->buffer; } -static av_always_inline unsigned int bytestream2_get_bytes_left_p(PutByteContext *p) +static av_always_inline int bytestream2_get_bytes_left_p(PutByteContext *p) { return p->buffer_end - p->buffer; }
bytestream2_get_bytes_left returns an unsigned int; as a result, it returns big positive numbers if an overread already happened, making it unsuitable for scenarios where one wants to allow this in a controlled way (because the buffer is actually padded so that no segfaults can happen). So change it to return an ordinary int. Also, bytestream2_get_bytes_left_p has been modified in the same way. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- Implemented James' suggestion to return an int. I have not found a place where this would pose a problem. libavcodec/bytestream.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)