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
--
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]
