[pkg-mad-maintainers] Bug#405801: [Fwd: Re: Regarding debian bug #405801.]

[Kurt Roeckx]

On Sat, Jan 13, 2007 at 06:45:36PM +0100, Andreas Henriksson wrote:
> libid3tag doesn't gracefully handle unexpected values in the
> files id3 encoding. Below is one such occation when id3_parse_uint
> apparently returns 50 (which I have no idea how it can be stored in an
> enum which doesn't contain a definition for 50).

Looking at the latest standard, the only valid encodings are 0 to 3.
50 is clearly wrong.

The field in question is TYER, which is the year, which is a "numeric
string", which should be encoded using latin1, so the encoding should
be set to 0.  It should also be 4 chars long, but it's only 3.  It
says "F+5".  This looks like a really broken tag.

Looking at the other tags, most of them don't seem to be making much
sense, some do.

> The "id3_parse_string" function doesn't have a default case in it's
> switch to catch this but (by accident?) happens to return NULL for this
> case. No error checking seems to be done in this particular caller to
> see if id3_parse_string returns NULL.

It does initialise ucs4 to NULL (0) in id3_parse_string(), and the code
there checks for it, the function that calls id3_parse_string() doesn't.

> Please verify for correctness! (The problem might be deeper, are we
> looking at the wrong byte in the file for the encoding? Am I just
> papering over a symptom of another bug?)

> diff -urip libid3tag-0.15.1b/compat.c libid3tag-0.15.1b.fixed/compat.c
> --- libid3tag-0.15.1b/compat.c	2004-02-17 03:34:39.000000000 +0100
> +++ libid3tag-0.15.1b.fixed/compat.c	2007-01-13 18:32:52.000000000 +0100
> @@ -442,6 +442,8 @@ int id3_compat_fixup(struct id3_tag *tag
>  
>      encoding = id3_parse_uint(&data, 1);
>      string   = id3_parse_string(&data, end - data, encoding, 0);
> +    if (string == NULL)
> +      goto fail;
>  
>      if (id3_ucs4_length(string) < 4) {
>        free(string);
> diff -urip libid3tag-0.15.1b/compat.gperf libid3tag-0.15.1b.fixed/compat.gperf
> --- libid3tag-0.15.1b/compat.gperf	2004-01-23 10:41:32.000000000 +0100
> +++ libid3tag-0.15.1b.fixed/compat.gperf	2007-01-13 18:33:20.000000000 +0100
> @@ -236,6 +236,8 @@ int id3_compat_fixup(struct id3_tag *tag
>  
>      encoding = id3_parse_uint(&data, 1);
>      string   = id3_parse_string(&data, end - data, encoding, 0);
> +    if (string == NULL)
> +      goto fail;
>  
>      if (id3_ucs4_length(string) < 4) {
>        free(string);

This part looks good.

> diff -urip libid3tag-0.15.1b/parse.c libid3tag-0.15.1b.fixed/parse.c
> --- libid3tag-0.15.1b/parse.c	2004-01-23 10:41:32.000000000 +0100
> +++ libid3tag-0.15.1b.fixed/parse.c	2007-01-13 18:35:42.000000000 +0100
> @@ -165,6 +165,9 @@ id3_ucs4_t *id3_parse_string(id3_byte_t 
>    case ID3_FIELD_TEXTENCODING_UTF_8:
>      ucs4 = id3_utf8_deserialize(ptr, length);
>      break;
> +  default:
> +    /* FIXME: Unknown encoding! Print warning? */
> +    return NULL;
>    }
>  
>    if (ucs4 && !full) {

This return NULL is useless, since it will already do that.


Kurt