[FFmpeg-devel] avformat/apetag: tag values are unsigned

Submitted by Oliver Collyer via ffmpeg-devel on April 19, 2019, 12:12 a.m.

Details

Message ID CAOYzH=KDMHWyd6Uzy6M1suugdEg9yOpNXa0yD4GwJAW2G3MQZw@mail.gmail.com
State New
Headers show

Commit Message

Oliver Collyer via ffmpeg-devel April 19, 2019, 12:12 a.m.
Fixes: UBSan runtime error
Found-by: Clusterfuzz
---
 libavformat/apetag.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

James Almer April 19, 2019, 12:25 a.m.
On 4/18/2019 9:12 PM, Dan Sanders via ffmpeg-devel wrote:
> Fixes: UBSan runtime error
> Found-by: Clusterfuzz
> ---
>  libavformat/apetag.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/libavformat/apetag.c b/libavformat/apetag.c
> index cdc602e1a9..2991f57d5d 100644
> --- a/libavformat/apetag.c
> +++ b/libavformat/apetag.c
> @@ -29,10 +29,10 @@
>  #include "apetag.h"
>  #include "internal.h"
> 
> -#define APE_TAG_FLAG_CONTAINS_HEADER  (1 << 31)
> -#define APE_TAG_FLAG_LACKS_FOOTER     (1 << 30)
> -#define APE_TAG_FLAG_IS_HEADER        (1 << 29)
> -#define APE_TAG_FLAG_IS_BINARY        (1 << 1)
> +#define APE_TAG_FLAG_CONTAINS_HEADER  (1U << 31)

Isn't it enough with this one only?

> +#define APE_TAG_FLAG_LACKS_FOOTER     (1U << 30)
> +#define APE_TAG_FLAG_IS_HEADER        (1U << 29)
> +#define APE_TAG_FLAG_IS_BINARY        (1U << 1)
> 
>  static int ape_tag_read_field(AVFormatContext *s)
>  {
>
Oliver Collyer via ffmpeg-devel April 19, 2019, 12:56 a.m.
> > +#define APE_TAG_FLAG_CONTAINS_HEADER  (1U << 31)
>
> Isn't it enough with this one only?

Yes, only APE_TAG_FLAG_CONTAINS_HEADER is problematic. I changed all
of them because the tags are only used in unsigned contexts anyway.
Michael Niedermayer April 19, 2019, 3:13 p.m.
On Thu, Apr 18, 2019 at 05:12:14PM -0700, Dan Sanders via ffmpeg-devel wrote:
> Fixes: UBSan runtime error
> Found-by: Clusterfuzz
> ---
>  libavformat/apetag.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/libavformat/apetag.c b/libavformat/apetag.c
> index cdc602e1a9..2991f57d5d 100644
> --- a/libavformat/apetag.c
> +++ b/libavformat/apetag.c
> @@ -29,10 +29,10 @@
>  #include "apetag.h"
>  #include "internal.h"
> 
> -#define APE_TAG_FLAG_CONTAINS_HEADER  (1 << 31)
> -#define APE_TAG_FLAG_LACKS_FOOTER     (1 << 30)
> -#define APE_TAG_FLAG_IS_HEADER        (1 << 29)
> -#define APE_TAG_FLAG_IS_BINARY        (1 << 1)
> +#define APE_TAG_FLAG_CONTAINS_HEADER  (1U << 31)
> +#define APE_TAG_FLAG_LACKS_FOOTER     (1U << 30)
> +#define APE_TAG_FLAG_IS_HEADER        (1U << 29)
> +#define APE_TAG_FLAG_IS_BINARY        (1U << 1)

LGTM (both with all changed as in the patch or just one changed)

whoever pushes please fix the Author/FROM
(not pushing myself as theres a discussion about changing only 1 #define)

thx

[...]
James Almer April 19, 2019, 4:26 p.m.
On 4/18/2019 9:56 PM, Dan Sanders via ffmpeg-devel wrote:
>>> +#define APE_TAG_FLAG_CONTAINS_HEADER  (1U << 31)
>>
>> Isn't it enough with this one only?
> 
> Yes, only APE_TAG_FLAG_CONTAINS_HEADER is problematic. I changed all
> of them because the tags are only used in unsigned contexts anyway.

Applied your patch changing only the above define, as the others are
unnecessary. Better for git blame as well.

Thanks.

Patch hide | download patch | download mbox

diff --git a/libavformat/apetag.c b/libavformat/apetag.c
index cdc602e1a9..2991f57d5d 100644
--- a/libavformat/apetag.c
+++ b/libavformat/apetag.c
@@ -29,10 +29,10 @@ 
 #include "apetag.h"
 #include "internal.h"

-#define APE_TAG_FLAG_CONTAINS_HEADER  (1 << 31)
-#define APE_TAG_FLAG_LACKS_FOOTER     (1 << 30)
-#define APE_TAG_FLAG_IS_HEADER        (1 << 29)
-#define APE_TAG_FLAG_IS_BINARY        (1 << 1)
+#define APE_TAG_FLAG_CONTAINS_HEADER  (1U << 31)
+#define APE_TAG_FLAG_LACKS_FOOTER     (1U << 30)
+#define APE_TAG_FLAG_IS_HEADER        (1U << 29)
+#define APE_TAG_FLAG_IS_BINARY        (1U << 1)

 static int ape_tag_read_field(AVFormatContext *s)
 {