Re: [FFmpeg-devel] [PATCH] libavutil/encryption_info: Add NULL checks.
On Tue, Jun 5, 2018 at 1:06 PM Mark Thompson wrote: > > On 05/06/18 17:30, Jacob Trimble wrote: > > Just because I can't check whether my food has salmonella doesn't mean > > I shouldn't check the temperature when I cook it. Adding a NULL check > > is trivial and will catch the most common error case. We also can't > > check whether malloc() allocates enough memory, so should we then not > > check for NULL? NULL is used as an error signal, so if the caller > > didn't include a NULL check, they will pass it here. Rather than > > crashing the program (hopefully it will crash, it is undefined > > behavior, so anything could happen), we should be nice and validate > > the input and error out. Just because it is impossible to check other > > error cases doesn't mean we should ignore all error checks. > > (My opinion, others may disagree.) > > Please consider what is actually useful to an API user here. > > The check you are suggesting will cause the function to, when passed entirely > invalid arguments, silently return having done nothing. Is this better than > the almost-guaranteed segfault you will get instead? Well, no. There is > much more scope for the error to go unnoticed and cause other hard-to-debug > issues later, where it could have been caught immediately. It returns an error, which should be checked by the caller. We can't do anything to change the caller's code, but we can make our code not crash their program. > > If there is a concern that a function like this could be misused then (since > this is certainly undefined behaviour in any case) turning it into an abort() > is the best case so that the program will definitely fail and any errors can > be diagnosed immediately. As such, I think argument checks for nonsensical > invalid input like this should be done either with av_assert or not at all. What about invalid MP4 files? Should we convert those to abort() too? When parsing MP4 files, we check that it is valid and return an error code if it is not. There is no difference between a method to parse an MP4 file and these, so these should validate the inputs as best it can and return errors if we find them. > > - Mark > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel When I design an API, I try to make it hard to misuse, being as helpful to the caller as possible. Having the function intentionally crash on certain input when a simple if could solve it is not a good idea. I think the memcpy API is an example of a bad api, it assumes valid input. Are we going to stop checking that MP4 files are valid too? If we only accept valid MP4 files it seems pointless to check it and return errors. These functions return NULL on errors, it is an error to pass NULL, therefore we should check and return NULL. It is impossible to check some things, but if it were possible, I would suggest checking that too. One other problem is that NULL is sometimes a valid value to pass to a function and sometimes not. NULL is used as a signal value whereas an invalid pointer is never valid. From the compiler's point of view, there is no difference between a function that accepts NULL as valid and one that does not. So the only thing we can do is write a comment saying "Does not accept NULL", but this is fragile as the compiler can't check this (without static analysis). By having us return an error for invalid input, it makes the API more stable and easier to use. We can't stop the caller from ignoring the return value, but we can at least do our best to validate the input and avoid crashing the program. But if you aren't going to accept a patch to increase code heath, there is nothing I can do. I'll remove the NULL checks from my other patch so it can be submitted. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libavutil/encryption_info: Add NULL checks.
On 05/06/18 17:30, Jacob Trimble wrote: > Just because I can't check whether my food has salmonella doesn't mean > I shouldn't check the temperature when I cook it. Adding a NULL check > is trivial and will catch the most common error case. We also can't > check whether malloc() allocates enough memory, so should we then not > check for NULL? NULL is used as an error signal, so if the caller > didn't include a NULL check, they will pass it here. Rather than > crashing the program (hopefully it will crash, it is undefined > behavior, so anything could happen), we should be nice and validate > the input and error out. Just because it is impossible to check other > error cases doesn't mean we should ignore all error checks. (My opinion, others may disagree.) Please consider what is actually useful to an API user here. The check you are suggesting will cause the function to, when passed entirely invalid arguments, silently return having done nothing. Is this better than the almost-guaranteed segfault you will get instead? Well, no. There is much more scope for the error to go unnoticed and cause other hard-to-debug issues later, where it could have been caught immediately. If there is a concern that a function like this could be misused then (since this is certainly undefined behaviour in any case) turning it into an abort() is the best case so that the program will definitely fail and any errors can be diagnosed immediately. As such, I think argument checks for nonsensical invalid input like this should be done either with av_assert or not at all. - Mark ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libavutil/encryption_info: Add NULL checks.
On Tue, Jun 05, 2018 at 09:30:51AM -0700, Jacob Trimble wrote:
> On Mon, Jun 4, 2018 at 2:24 PM Carl Eugen Hoyos wrote:
> >
> > 2018-06-04 23:07 GMT+02:00, Jacob Trimble
> > :
> > > On Mon, Jun 4, 2018 at 10:46 AM Carl Eugen Hoyos
> > > wrote:
> > >>
> > >> 2018-06-04 18:59 GMT+02:00, Jacob Trimble
> > >> :
> > >> > On Fri, Jun 1, 2018 at 5:03 PM Michael Niedermayer
> > >> > wrote:
> > >> >>
> > >> >> On Thu, May 31, 2018 at 09:33:36AM -0700, Jacob Trimble wrote:
> > >> >> > Found by Chrome's ClusterFuzz: http://crbug.com/846662.
> > >> >> >
> > >> >> > Signed-off-by: Jacob Trimble
> > >> >> > ---
> > >> >> > libavutil/encryption_info.c | 7 +--
> > >> >> > 1 file changed, 5 insertions(+), 2 deletions(-)
> > >> >> >
> > >> >> > diff --git a/libavutil/encryption_info.c
> > >> >> > b/libavutil/encryption_info.c
> > >> >> > index 20a752d6b4..a48ded922c 100644
> > >> >> > --- a/libavutil/encryption_info.c
> > >> >> > +++ b/libavutil/encryption_info.c
> > >> >> > @@ -64,6 +64,8 @@ AVEncryptionInfo *av_encryption_info_clone(const
> > >> >> > AVEncryptionInfo *info)
> > >> >> > {
> > >> >> > AVEncryptionInfo *ret;
> > >> >> >
> > >> >> > +if (!info)
> > >> >> > +return NULL;
> > >> >> > ret = av_encryption_info_alloc(info->subsample_count,
> > >> >> > info->key_id_size, info->iv_size);
> > >> >> > if (!ret)
> > >> >> > return NULL;
> > >> >>
> > >> >> > @@ -127,7 +129,7 @@ uint8_t *av_encryption_info_add_side_data(const
> > >> >> > AVEncryptionInfo *info, size_t *
> > >> >> > uint8_t *buffer, *cur_buffer;
> > >> >> > uint32_t i;
> > >> >> >
> > >> >> > -if (UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA < info->key_id_size
> > >> >> > ||
> > >> >> > +if (!info || !size || UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA <
> > >> >> > info->key_id_size ||
> > >> >> > UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA - info->key_id_size <
> > >> >> > info->iv_size ||
> > >> >> > (UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA - info->key_id_size
> > >> >> > -
> > >> >> > info->iv_size) / 8 < info->subsample_count) {
> > >> >> > return NULL;
> > >> >> > @@ -260,7 +262,8 @@ uint8_t
> > >> >> > *av_encryption_init_info_add_side_data(const
> > >> >> > AVEncryptionInitInfo *info,
> > >> >> > uint8_t *buffer, *cur_buffer;
> > >> >> > uint32_t i, max_size;
> > >> >> >
> > >> >> > -if (UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA <
> > >> >> > info->system_id_size ||
> > >> >> > +if (!info || !side_data_size ||
> > >> >> > +UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA <
> > >> >> > info->system_id_size ||
> > >> >> > UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA -
> > >> >> > info->system_id_size < info->data_size) {
> > >> >> > return NULL;
> > >> >> > }
> > >> >>
> > >> >> in which valid case would these be called with NULL input ?
> > >> >> iam asking as this feels as if it might be a bug in teh caller
> > >> >>
> > >> >
> > >> > This was found by Chrome's ClusterFuzz, which I am not that familiar
> > >> > with. I think it was just running fuzz tests directly on FFmpeg code,
> > >> > so it wasn't in production code. But since this is a public method,
> > >> > we should validate the input in any case.
> > >>
> > >> How do you validate the size of C buffers in general?
> > >
> > > I'm not sure I understand your comment. You can't verify the
> > > length of buffers unless the size is given to the method.
> >
> > If we can't verify the size of the buffer (where both overread
> > and overwrite at least can have catastrophic impact) why
> > is it a good idea to check if the user passed an actual pointer
> > (as is required) or NULL as argument (where NULL typically
> > has limited impact)?
> >
> > > These functions do accept the size and verify that the data
> > > is valid for the given size.
> >
> > I may misunderstand the code but it looks to me as if the
> > given size is only checked because the needed space is
> > not necessarily known in advance / most functions do not
> > check.
>
> This method doesn't need the size at all, the number of elements is
> actually encoded in the side-data. These methods use the
> side_data_size to double-check that the number of bytes is large
> enough to hold the number of elements that the side-data says there
> are.
>
> >
> > > Since we are verifying the data and the size we are
> > > given, we should be checking for NULL as well.
> >
> > Why?
> > (As we cannot check for the worse case anyway.)
>
> Just because I can't check whether my food has salmonella doesn't mean
> I shouldn't check the temperature when I cook it. Adding a NULL check
> is trivial and will catch the most common error case. We also can't
> check whether malloc() allocates enough memory, so should we then not
> check for NULL? NULL is used as an error signal, so if the caller
> didn't include a NULL check, they will pass it here. Rather than
> crashing the program (hopefully it will crash, it is undefined
>
Re: [FFmpeg-devel] [PATCH] libavutil/encryption_info: Add NULL checks.
On Mon, Jun 4, 2018 at 2:24 PM Carl Eugen Hoyos wrote:
>
> 2018-06-04 23:07 GMT+02:00, Jacob Trimble :
> > On Mon, Jun 4, 2018 at 10:46 AM Carl Eugen Hoyos wrote:
> >>
> >> 2018-06-04 18:59 GMT+02:00, Jacob Trimble
> >> :
> >> > On Fri, Jun 1, 2018 at 5:03 PM Michael Niedermayer
> >> > wrote:
> >> >>
> >> >> On Thu, May 31, 2018 at 09:33:36AM -0700, Jacob Trimble wrote:
> >> >> > Found by Chrome's ClusterFuzz: http://crbug.com/846662.
> >> >> >
> >> >> > Signed-off-by: Jacob Trimble
> >> >> > ---
> >> >> > libavutil/encryption_info.c | 7 +--
> >> >> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >> >> >
> >> >> > diff --git a/libavutil/encryption_info.c
> >> >> > b/libavutil/encryption_info.c
> >> >> > index 20a752d6b4..a48ded922c 100644
> >> >> > --- a/libavutil/encryption_info.c
> >> >> > +++ b/libavutil/encryption_info.c
> >> >> > @@ -64,6 +64,8 @@ AVEncryptionInfo *av_encryption_info_clone(const
> >> >> > AVEncryptionInfo *info)
> >> >> > {
> >> >> > AVEncryptionInfo *ret;
> >> >> >
> >> >> > +if (!info)
> >> >> > +return NULL;
> >> >> > ret = av_encryption_info_alloc(info->subsample_count,
> >> >> > info->key_id_size, info->iv_size);
> >> >> > if (!ret)
> >> >> > return NULL;
> >> >>
> >> >> > @@ -127,7 +129,7 @@ uint8_t *av_encryption_info_add_side_data(const
> >> >> > AVEncryptionInfo *info, size_t *
> >> >> > uint8_t *buffer, *cur_buffer;
> >> >> > uint32_t i;
> >> >> >
> >> >> > -if (UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA < info->key_id_size ||
> >> >> > +if (!info || !size || UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA <
> >> >> > info->key_id_size ||
> >> >> > UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA - info->key_id_size <
> >> >> > info->iv_size ||
> >> >> > (UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA - info->key_id_size -
> >> >> > info->iv_size) / 8 < info->subsample_count) {
> >> >> > return NULL;
> >> >> > @@ -260,7 +262,8 @@ uint8_t
> >> >> > *av_encryption_init_info_add_side_data(const
> >> >> > AVEncryptionInitInfo *info,
> >> >> > uint8_t *buffer, *cur_buffer;
> >> >> > uint32_t i, max_size;
> >> >> >
> >> >> > -if (UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA <
> >> >> > info->system_id_size ||
> >> >> > +if (!info || !side_data_size ||
> >> >> > +UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA <
> >> >> > info->system_id_size ||
> >> >> > UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA -
> >> >> > info->system_id_size < info->data_size) {
> >> >> > return NULL;
> >> >> > }
> >> >>
> >> >> in which valid case would these be called with NULL input ?
> >> >> iam asking as this feels as if it might be a bug in teh caller
> >> >>
> >> >
> >> > This was found by Chrome's ClusterFuzz, which I am not that familiar
> >> > with. I think it was just running fuzz tests directly on FFmpeg code,
> >> > so it wasn't in production code. But since this is a public method,
> >> > we should validate the input in any case.
> >>
> >> How do you validate the size of C buffers in general?
> >
> > I'm not sure I understand your comment. You can't verify the
> > length of buffers unless the size is given to the method.
>
> If we can't verify the size of the buffer (where both overread
> and overwrite at least can have catastrophic impact) why
> is it a good idea to check if the user passed an actual pointer
> (as is required) or NULL as argument (where NULL typically
> has limited impact)?
>
> > These functions do accept the size and verify that the data
> > is valid for the given size.
>
> I may misunderstand the code but it looks to me as if the
> given size is only checked because the needed space is
> not necessarily known in advance / most functions do not
> check.
This method doesn't need the size at all, the number of elements is
actually encoded in the side-data. These methods use the
side_data_size to double-check that the number of bytes is large
enough to hold the number of elements that the side-data says there
are.
>
> > Since we are verifying the data and the size we are
> > given, we should be checking for NULL as well.
>
> Why?
> (As we cannot check for the worse case anyway.)
Just because I can't check whether my food has salmonella doesn't mean
I shouldn't check the temperature when I cook it. Adding a NULL check
is trivial and will catch the most common error case. We also can't
check whether malloc() allocates enough memory, so should we then not
check for NULL? NULL is used as an error signal, so if the caller
didn't include a NULL check, they will pass it here. Rather than
crashing the program (hopefully it will crash, it is undefined
behavior, so anything could happen), we should be nice and validate
the input and error out. Just because it is impossible to check other
error cases doesn't mean we should ignore all error checks.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
Re: [FFmpeg-devel] [PATCH] libavutil/encryption_info: Add NULL checks.
2018-06-04 23:07 GMT+02:00, Jacob Trimble :
> On Mon, Jun 4, 2018 at 10:46 AM Carl Eugen Hoyos wrote:
>>
>> 2018-06-04 18:59 GMT+02:00, Jacob Trimble
>> :
>> > On Fri, Jun 1, 2018 at 5:03 PM Michael Niedermayer
>> > wrote:
>> >>
>> >> On Thu, May 31, 2018 at 09:33:36AM -0700, Jacob Trimble wrote:
>> >> > Found by Chrome's ClusterFuzz: http://crbug.com/846662.
>> >> >
>> >> > Signed-off-by: Jacob Trimble
>> >> > ---
>> >> > libavutil/encryption_info.c | 7 +--
>> >> > 1 file changed, 5 insertions(+), 2 deletions(-)
>> >> >
>> >> > diff --git a/libavutil/encryption_info.c
>> >> > b/libavutil/encryption_info.c
>> >> > index 20a752d6b4..a48ded922c 100644
>> >> > --- a/libavutil/encryption_info.c
>> >> > +++ b/libavutil/encryption_info.c
>> >> > @@ -64,6 +64,8 @@ AVEncryptionInfo *av_encryption_info_clone(const
>> >> > AVEncryptionInfo *info)
>> >> > {
>> >> > AVEncryptionInfo *ret;
>> >> >
>> >> > +if (!info)
>> >> > +return NULL;
>> >> > ret = av_encryption_info_alloc(info->subsample_count,
>> >> > info->key_id_size, info->iv_size);
>> >> > if (!ret)
>> >> > return NULL;
>> >>
>> >> > @@ -127,7 +129,7 @@ uint8_t *av_encryption_info_add_side_data(const
>> >> > AVEncryptionInfo *info, size_t *
>> >> > uint8_t *buffer, *cur_buffer;
>> >> > uint32_t i;
>> >> >
>> >> > -if (UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA < info->key_id_size ||
>> >> > +if (!info || !size || UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA <
>> >> > info->key_id_size ||
>> >> > UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA - info->key_id_size <
>> >> > info->iv_size ||
>> >> > (UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA - info->key_id_size -
>> >> > info->iv_size) / 8 < info->subsample_count) {
>> >> > return NULL;
>> >> > @@ -260,7 +262,8 @@ uint8_t
>> >> > *av_encryption_init_info_add_side_data(const
>> >> > AVEncryptionInitInfo *info,
>> >> > uint8_t *buffer, *cur_buffer;
>> >> > uint32_t i, max_size;
>> >> >
>> >> > -if (UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA <
>> >> > info->system_id_size ||
>> >> > +if (!info || !side_data_size ||
>> >> > +UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA <
>> >> > info->system_id_size ||
>> >> > UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA -
>> >> > info->system_id_size < info->data_size) {
>> >> > return NULL;
>> >> > }
>> >>
>> >> in which valid case would these be called with NULL input ?
>> >> iam asking as this feels as if it might be a bug in teh caller
>> >>
>> >
>> > This was found by Chrome's ClusterFuzz, which I am not that familiar
>> > with. I think it was just running fuzz tests directly on FFmpeg code,
>> > so it wasn't in production code. But since this is a public method,
>> > we should validate the input in any case.
>>
>> How do you validate the size of C buffers in general?
>
> I'm not sure I understand your comment. You can't verify the
> length of buffers unless the size is given to the method.
If we can't verify the size of the buffer (where both overread
and overwrite at least can have catastrophic impact) why
is it a good idea to check if the user passed an actual pointer
(as is required) or NULL as argument (where NULL typically
has limited impact)?
> These functions do accept the size and verify that the data
> is valid for the given size.
I may misunderstand the code but it looks to me as if the
given size is only checked because the needed space is
not necessarily known in advance / most functions do not
check.
> Since we are verifying the data and the size we are
> given, we should be checking for NULL as well.
Why?
(As we cannot check for the worse case anyway.)
Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libavutil/encryption_info: Add NULL checks.
On Mon, Jun 4, 2018 at 10:46 AM Carl Eugen Hoyos wrote:
>
> 2018-06-04 18:59 GMT+02:00, Jacob Trimble :
> > On Fri, Jun 1, 2018 at 5:03 PM Michael Niedermayer
> > wrote:
> >>
> >> On Thu, May 31, 2018 at 09:33:36AM -0700, Jacob Trimble wrote:
> >> > Found by Chrome's ClusterFuzz: http://crbug.com/846662.
> >> >
> >> > Signed-off-by: Jacob Trimble
> >> > ---
> >> > libavutil/encryption_info.c | 7 +--
> >> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/libavutil/encryption_info.c b/libavutil/encryption_info.c
> >> > index 20a752d6b4..a48ded922c 100644
> >> > --- a/libavutil/encryption_info.c
> >> > +++ b/libavutil/encryption_info.c
> >> > @@ -64,6 +64,8 @@ AVEncryptionInfo *av_encryption_info_clone(const
> >> > AVEncryptionInfo *info)
> >> > {
> >> > AVEncryptionInfo *ret;
> >> >
> >> > +if (!info)
> >> > +return NULL;
> >> > ret = av_encryption_info_alloc(info->subsample_count,
> >> > info->key_id_size, info->iv_size);
> >> > if (!ret)
> >> > return NULL;
> >>
> >> > @@ -127,7 +129,7 @@ uint8_t *av_encryption_info_add_side_data(const
> >> > AVEncryptionInfo *info, size_t *
> >> > uint8_t *buffer, *cur_buffer;
> >> > uint32_t i;
> >> >
> >> > -if (UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA < info->key_id_size ||
> >> > +if (!info || !size || UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA <
> >> > info->key_id_size ||
> >> > UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA - info->key_id_size <
> >> > info->iv_size ||
> >> > (UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA - info->key_id_size -
> >> > info->iv_size) / 8 < info->subsample_count) {
> >> > return NULL;
> >> > @@ -260,7 +262,8 @@ uint8_t *av_encryption_init_info_add_side_data(const
> >> > AVEncryptionInitInfo *info,
> >> > uint8_t *buffer, *cur_buffer;
> >> > uint32_t i, max_size;
> >> >
> >> > -if (UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA <
> >> > info->system_id_size ||
> >> > +if (!info || !side_data_size ||
> >> > +UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA <
> >> > info->system_id_size ||
> >> > UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA -
> >> > info->system_id_size < info->data_size) {
> >> > return NULL;
> >> > }
> >>
> >> in which valid case would these be called with NULL input ?
> >> iam asking as this feels as if it might be a bug in teh caller
> >>
> >
> > This was found by Chrome's ClusterFuzz, which I am not that familiar
> > with. I think it was just running fuzz tests directly on FFmpeg code,
> > so it wasn't in production code. But since this is a public method,
> > we should validate the input in any case.
>
> How do you validate the size of C buffers in general?
>
I'm not sure I understand your comment. You can't verify the length
of buffers unless the size is given to the method. These functions do
accept the size and verify that the data is valid for the given size.
Since we are verifying the data and the size we are given, we should
be checking for NULL as well.
> Carl Eugen
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libavutil/encryption_info: Add NULL checks.
2018-06-04 18:59 GMT+02:00, Jacob Trimble :
> On Fri, Jun 1, 2018 at 5:03 PM Michael Niedermayer
> wrote:
>>
>> On Thu, May 31, 2018 at 09:33:36AM -0700, Jacob Trimble wrote:
>> > Found by Chrome's ClusterFuzz: http://crbug.com/846662.
>> >
>> > Signed-off-by: Jacob Trimble
>> > ---
>> > libavutil/encryption_info.c | 7 +--
>> > 1 file changed, 5 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/libavutil/encryption_info.c b/libavutil/encryption_info.c
>> > index 20a752d6b4..a48ded922c 100644
>> > --- a/libavutil/encryption_info.c
>> > +++ b/libavutil/encryption_info.c
>> > @@ -64,6 +64,8 @@ AVEncryptionInfo *av_encryption_info_clone(const
>> > AVEncryptionInfo *info)
>> > {
>> > AVEncryptionInfo *ret;
>> >
>> > +if (!info)
>> > +return NULL;
>> > ret = av_encryption_info_alloc(info->subsample_count,
>> > info->key_id_size, info->iv_size);
>> > if (!ret)
>> > return NULL;
>>
>> > @@ -127,7 +129,7 @@ uint8_t *av_encryption_info_add_side_data(const
>> > AVEncryptionInfo *info, size_t *
>> > uint8_t *buffer, *cur_buffer;
>> > uint32_t i;
>> >
>> > -if (UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA < info->key_id_size ||
>> > +if (!info || !size || UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA <
>> > info->key_id_size ||
>> > UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA - info->key_id_size <
>> > info->iv_size ||
>> > (UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA - info->key_id_size -
>> > info->iv_size) / 8 < info->subsample_count) {
>> > return NULL;
>> > @@ -260,7 +262,8 @@ uint8_t *av_encryption_init_info_add_side_data(const
>> > AVEncryptionInitInfo *info,
>> > uint8_t *buffer, *cur_buffer;
>> > uint32_t i, max_size;
>> >
>> > -if (UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA <
>> > info->system_id_size ||
>> > +if (!info || !side_data_size ||
>> > +UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA <
>> > info->system_id_size ||
>> > UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA -
>> > info->system_id_size < info->data_size) {
>> > return NULL;
>> > }
>>
>> in which valid case would these be called with NULL input ?
>> iam asking as this feels as if it might be a bug in teh caller
>>
>
> This was found by Chrome's ClusterFuzz, which I am not that familiar
> with. I think it was just running fuzz tests directly on FFmpeg code,
> so it wasn't in production code. But since this is a public method,
> we should validate the input in any case.
How do you validate the size of C buffers in general?
Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libavutil/encryption_info: Add NULL checks.
On Fri, Jun 1, 2018 at 5:03 PM Michael Niedermayer
wrote:
>
> On Thu, May 31, 2018 at 09:33:36AM -0700, Jacob Trimble wrote:
> > Found by Chrome's ClusterFuzz: http://crbug.com/846662.
> >
> > Signed-off-by: Jacob Trimble
> > ---
> > libavutil/encryption_info.c | 7 +--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavutil/encryption_info.c b/libavutil/encryption_info.c
> > index 20a752d6b4..a48ded922c 100644
> > --- a/libavutil/encryption_info.c
> > +++ b/libavutil/encryption_info.c
> > @@ -64,6 +64,8 @@ AVEncryptionInfo *av_encryption_info_clone(const
> > AVEncryptionInfo *info)
> > {
> > AVEncryptionInfo *ret;
> >
> > +if (!info)
> > +return NULL;
> > ret = av_encryption_info_alloc(info->subsample_count,
> > info->key_id_size, info->iv_size);
> > if (!ret)
> > return NULL;
>
> > @@ -127,7 +129,7 @@ uint8_t *av_encryption_info_add_side_data(const
> > AVEncryptionInfo *info, size_t *
> > uint8_t *buffer, *cur_buffer;
> > uint32_t i;
> >
> > -if (UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA < info->key_id_size ||
> > +if (!info || !size || UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA <
> > info->key_id_size ||
> > UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA - info->key_id_size <
> > info->iv_size ||
> > (UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA - info->key_id_size -
> > info->iv_size) / 8 < info->subsample_count) {
> > return NULL;
> > @@ -260,7 +262,8 @@ uint8_t *av_encryption_init_info_add_side_data(const
> > AVEncryptionInitInfo *info,
> > uint8_t *buffer, *cur_buffer;
> > uint32_t i, max_size;
> >
> > -if (UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA < info->system_id_size
> > ||
> > +if (!info || !side_data_size ||
> > +UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA < info->system_id_size
> > ||
> > UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA - info->system_id_size
> > < info->data_size) {
> > return NULL;
> > }
>
> in which valid case would these be called with NULL input ?
> iam asking as this feels as if it might be a bug in teh caller
>
This was found by Chrome's ClusterFuzz, which I am not that familiar
with. I think it was just running fuzz tests directly on FFmpeg code,
so it wasn't in production code. But since this is a public method,
we should validate the input in any case.
> thx
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> In a rich man's house there is no place to spit but his face.
> -- Diogenes of Sinope
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libavutil/encryption_info: Add NULL checks.
On Thu, May 31, 2018 at 09:33:36AM -0700, Jacob Trimble wrote:
> Found by Chrome's ClusterFuzz: http://crbug.com/846662.
>
> Signed-off-by: Jacob Trimble
> ---
> libavutil/encryption_info.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/libavutil/encryption_info.c b/libavutil/encryption_info.c
> index 20a752d6b4..a48ded922c 100644
> --- a/libavutil/encryption_info.c
> +++ b/libavutil/encryption_info.c
> @@ -64,6 +64,8 @@ AVEncryptionInfo *av_encryption_info_clone(const
> AVEncryptionInfo *info)
> {
> AVEncryptionInfo *ret;
>
> +if (!info)
> +return NULL;
> ret = av_encryption_info_alloc(info->subsample_count, info->key_id_size,
> info->iv_size);
> if (!ret)
> return NULL;
> @@ -127,7 +129,7 @@ uint8_t *av_encryption_info_add_side_data(const
> AVEncryptionInfo *info, size_t *
> uint8_t *buffer, *cur_buffer;
> uint32_t i;
>
> -if (UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA < info->key_id_size ||
> +if (!info || !size || UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA <
> info->key_id_size ||
> UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA - info->key_id_size <
> info->iv_size ||
> (UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA - info->key_id_size -
> info->iv_size) / 8 < info->subsample_count) {
> return NULL;
> @@ -260,7 +262,8 @@ uint8_t *av_encryption_init_info_add_side_data(const
> AVEncryptionInitInfo *info,
> uint8_t *buffer, *cur_buffer;
> uint32_t i, max_size;
>
> -if (UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA < info->system_id_size ||
> +if (!info || !side_data_size ||
> +UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA < info->system_id_size ||
> UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA - info->system_id_size <
> info->data_size) {
> return NULL;
> }
in which valid case would these be called with NULL input ?
iam asking as this feels as if it might be a bug in teh caller
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] libavutil/encryption_info: Add NULL checks.
Found by Chrome's ClusterFuzz: http://crbug.com/846662.
Signed-off-by: Jacob Trimble
---
libavutil/encryption_info.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/libavutil/encryption_info.c b/libavutil/encryption_info.c
index 20a752d6b4..a48ded922c 100644
--- a/libavutil/encryption_info.c
+++ b/libavutil/encryption_info.c
@@ -64,6 +64,8 @@ AVEncryptionInfo *av_encryption_info_clone(const
AVEncryptionInfo *info)
{
AVEncryptionInfo *ret;
+if (!info)
+return NULL;
ret = av_encryption_info_alloc(info->subsample_count, info->key_id_size,
info->iv_size);
if (!ret)
return NULL;
@@ -127,7 +129,7 @@ uint8_t *av_encryption_info_add_side_data(const
AVEncryptionInfo *info, size_t *
uint8_t *buffer, *cur_buffer;
uint32_t i;
-if (UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA < info->key_id_size ||
+if (!info || !size || UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA <
info->key_id_size ||
UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA - info->key_id_size <
info->iv_size ||
(UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA - info->key_id_size -
info->iv_size) / 8 < info->subsample_count) {
return NULL;
@@ -260,7 +262,8 @@ uint8_t *av_encryption_init_info_add_side_data(const
AVEncryptionInitInfo *info,
uint8_t *buffer, *cur_buffer;
uint32_t i, max_size;
-if (UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA < info->system_id_size ||
+if (!info || !side_data_size ||
+UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA < info->system_id_size ||
UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA - info->system_id_size <
info->data_size) {
return NULL;
}
--
2.17.0.921.gf22659ad46-goog
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
