Re: [FFmpeg-devel] [PATCH 2/2] libavformat/takdec.c: Fix msan error.

Re: [FFmpeg-devel] [PATCH 2/2] libavformat/takdec.c: Fix msan error.

[Thierry Foucu]

On Mon, Apr 7, 2025 at 11:10 AM <tfoucu@google.com> wrote:

> From: Thierry Foucu <tfoucu@gmail.com>
>
> Make sure we are reading 16 bytes for the MD5
> ---
>  libavformat/takdec.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/takdec.c b/libavformat/takdec.c
> index 21fff3fcbf..61b9f001c4 100644
> --- a/libavformat/takdec.c
> +++ b/libavformat/takdec.c
> @@ -19,6 +19,7 @@
>   * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301 USA
>   */
>
> +#include <libavutil/error.h>
>  #include "libavutil/crc.h"
>  #include "libavutil/mem.h"
>
> @@ -116,7 +117,9 @@ static int tak_read_header(AVFormatContext *s)
>              if (size != 19)
>                  return AVERROR_INVALIDDATA;
>              ffio_init_checksum(pb, tak_check_crc, 0xCE04B7U);
> -            avio_read(pb, md5, 16);
> +            if (avio_read(pb, md5, 16) != 16) {
> +                return AVERROR(EIO);
> +            }
>              if (ffio_get_checksum(s->pb) != avio_rb24(pb)) {
>                  av_log(s, AV_LOG_ERROR, "MD5 metadata block CRC
> error.\n");
>                  if (s->error_recognition & AV_EF_EXPLODE)
> --
> 2.49.0.504.g3bcea36a83-goog
>
>
Ping?
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] libavformat/takdec.c: Fix msan error.

[James Almer]

[-- Attachment #1.1.1: Type: text/plain, Size: 1811 bytes --]

On 4/17/2025 4:08 PM, Thierry Foucu wrote:
> On Mon, Apr 7, 2025 at 11:10 AM <tfoucu@google.com> wrote:
> 
>> From: Thierry Foucu <tfoucu@gmail.com>
>>
>> Make sure we are reading 16 bytes for the MD5
>> ---
>>   libavformat/takdec.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavformat/takdec.c b/libavformat/takdec.c
>> index 21fff3fcbf..61b9f001c4 100644
>> --- a/libavformat/takdec.c
>> +++ b/libavformat/takdec.c
>> @@ -19,6 +19,7 @@
>>    * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
>> 02110-1301 USA
>>    */
>>
>> +#include <libavutil/error.h>
>>   #include "libavutil/crc.h"
>>   #include "libavutil/mem.h"
>>
>> @@ -116,7 +117,9 @@ static int tak_read_header(AVFormatContext *s)
>>               if (size != 19)
>>                   return AVERROR_INVALIDDATA;
>>               ffio_init_checksum(pb, tak_check_crc, 0xCE04B7U);
>> -            avio_read(pb, md5, 16);
>> +            if (avio_read(pb, md5, 16) != 16) {
>> +                return AVERROR(EIO);

Should be AVERROR_INVALIDDATA imo, since i means the file is just 
truncated. It wasn't an error in the protocol reading data.

Will amend with that change and push. Thanks.

>> +            }
>>               if (ffio_get_checksum(s->pb) != avio_rb24(pb)) {
>>                   av_log(s, AV_LOG_ERROR, "MD5 metadata block CRC
>> error.\n");
>>                   if (s->error_recognition & AV_EF_EXPLODE)
>> --
>> 2.49.0.504.g3bcea36a83-goog
>>
>>
> Ping?
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] libavformat/takdec.c: Fix msan error.

[Thierry Foucu]

Thanks James

On Thu, Apr 17, 2025 at 1:25 PM James Almer <jamrial@gmail.com> wrote:

> On 4/17/2025 4:08 PM, Thierry Foucu wrote:
> > On Mon, Apr 7, 2025 at 11:10 AM <tfoucu@google.com> wrote:
> >
> >> From: Thierry Foucu <tfoucu@gmail.com>
> >>
> >> Make sure we are reading 16 bytes for the MD5
> >> ---
> >>   libavformat/takdec.c | 5 ++++-
> >>   1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/libavformat/takdec.c b/libavformat/takdec.c
> >> index 21fff3fcbf..61b9f001c4 100644
> >> --- a/libavformat/takdec.c
> >> +++ b/libavformat/takdec.c
> >> @@ -19,6 +19,7 @@
> >>    * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> >> 02110-1301 USA
> >>    */
> >>
> >> +#include <libavutil/error.h>
> >>   #include "libavutil/crc.h"
> >>   #include "libavutil/mem.h"
> >>
> >> @@ -116,7 +117,9 @@ static int tak_read_header(AVFormatContext *s)
> >>               if (size != 19)
> >>                   return AVERROR_INVALIDDATA;
> >>               ffio_init_checksum(pb, tak_check_crc, 0xCE04B7U);
> >> -            avio_read(pb, md5, 16);
> >> +            if (avio_read(pb, md5, 16) != 16) {
> >> +                return AVERROR(EIO);
>
> Should be AVERROR_INVALIDDATA imo, since i means the file is just
> truncated. It wasn't an error in the protocol reading data.
>
>
Regarding the error message, I was trying to be consistent with the other
avio_read error, like here:
https://github.com/FFmpeg/FFmpeg/blob/master/libavformat/takdec.c#L99


> Will amend with that change and push. Thanks.
>
> >> +            }
> >>               if (ffio_get_checksum(s->pb) != avio_rb24(pb)) {
> >>                   av_log(s, AV_LOG_ERROR, "MD5 metadata block CRC
> >> error.\n");
> >>                   if (s->error_recognition & AV_EF_EXPLODE)
> >> --
> >> 2.49.0.504.g3bcea36a83-goog
> >>
> >>
> > Ping?
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>


-- 

Thierry Foucu
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] libavformat/takdec.c: Fix msan error.

[Nicolas George]

Thierry Foucu (HE12025-04-21):
> > >> -            avio_read(pb, md5, 16);
> > >> +            if (avio_read(pb, md5, 16) != 16) {
> > >> +                return AVERROR(EIO);
> > Should be AVERROR_INVALIDDATA imo, since i means the file is just
> > truncated. It wasn't an error in the protocol reading data.
> Regarding the error message, I was trying to be consistent with the other
> avio_read error, like here:
> https://github.com/FFmpeg/FFmpeg/blob/master/libavformat/takdec.c#L99

Both codes are wrong: if avio_read() returns an error, it must be
returned as is, not a new error invented.

And if avio_read() returns less than requested, then I agree with James,
INVALID_DATA is more correct than EIO. In both places.

Regards,

-- 
  Nicolas George
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] libavformat/takdec.c: Fix msan error.

[Thierry Foucu]

On Mon, Apr 21, 2025 at 10:01 AM Nicolas George <george@nsup.org> wrote:

> Thierry Foucu (HE12025-04-21):
> > > >> -            avio_read(pb, md5, 16);
> > > >> +            if (avio_read(pb, md5, 16) != 16) {
> > > >> +                return AVERROR(EIO);
> > > Should be AVERROR_INVALIDDATA imo, since i means the file is just
> > > truncated. It wasn't an error in the protocol reading data.
> > Regarding the error message, I was trying to be consistent with the other
> > avio_read error, like here:
> > https://github.com/FFmpeg/FFmpeg/blob/master/libavformat/takdec.c#L99
>
> Both codes are wrong: if avio_read() returns an error, it must be
> returned as is, not a new error invented.
>
> And if avio_read() returns less than requested, then I agree with James,
> INVALID_DATA is more correct than EIO. In both places.
>

Thanks. This is good to know..


>
> Regards,
>
> --
>   Nicolas George
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>


-- 

Thierry Foucu
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".