[Lame-dev] [ lame-Bugs-1636267 ] ID3v2 tags overwritten

[SourceForge.net <no...@so...>]

Bugs item #1636267, was opened at 2007-01-15 17:11
Message generated for change (Comment added) made by jstottsj
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=100290&aid=1636267&group_id=290

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: Libraries
Group: None
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: Jonathan Stott (jstottsj)
Assigned to: Nobody/Anonymous (nobody)
Summary: ID3v2 tags overwritten

Initial Comment:
If the output file is opened write-only instead of read-write, the ID3v2 tags will be silently overwritten by the VBR/XING tag.  This is true for both the 3.97 release and the current CVS snapshot.

The problem is the two freads() in PutVbrTag [VbrTag.c, lines 837 and 857 in the CVS snapshot].  The two fread()'s do not check their return code and assume the read was successful.  They then look at the data returned to determine if there are ID3v2 tags at the start of the file.

If the file was opened write-only, though, the reads will always fail.  Since the return code is not checked, the program interprets this to mean that there is no ID3v2 tag at the start of the file, and the promptly over-writes that data with the XING header.  Since this is the ONLY place that libmp3lame.a reads the data file, it's VERY easy to miss this unless you're looking for the v2 tags specifically.

Since the resulting file is an other-wise valid MP3 file and the problem occurs only after encoding the entire file, I would suggest changing the code so that:

1) It checks the return code of the fread() and
2) If the fread() fails, print a warning message ("Can't read from file, write-only file?" or something similar) and then over-write the region as usual.  It seems bad form to abort the encoding at the very end when everything else is valid data.

-Jonathan


----------------------------------------------------------------------

>Comment By: Jonathan Stott (jstottsj)
Date: 2007-02-18 22:25

Message:
Logged In: YES 
user_id=1692900
Originator: YES

Wend faster than I thought.  Minimal patch is attached.  Doesn't print any
error messages, doesn't cache any data sizes, just checks return values and
returns -1 if there's an error

-JS

PS The patch is against v3.97, not the nightly build.
File Added: id3v2_bug.patch

----------------------------------------------------------------------

Comment By: Jonathan Stott (jstottsj)
Date: 2007-02-18 22:13

Message:
Logged In: YES 
user_id=1692900
Originator: YES

Caching the size of the tags already written would work too, but as you
say it makes assumptions about how the data is being used.  Right now,
PutVbrTag() is assuming:
  1) The file is read-write,
  2) The file is seekable, and
  3) The VBR data can be written at the start of an already-opened file.
These assumptions are not documented in the release (v3.97) source code
for the function and return codes of these functions are not checked (its
assumed that they always succeed).

Caching the file offset eliminates assumtion #1.  This remains an issue if
the user isn't writing the returned data to the start of a file, but that's
already an issue for the PutVbrTag() function.

I guess I'm saying that it would work, but I think checking libc return
values and passing back a relevant error value might be more useful.

I'll see if I can't post a patch in the next day or two for you to look
at.

-Jonathan


----------------------------------------------------------------------

Comment By: Steve Fosdick (steve-fosdick)
Date: 2007-02-18 18:09

Message:
Logged In: YES 
user_id=1722388
Originator: NO

I don't know the code so I don't know how practical it would be but the
other possible explanation that occurs to be would be for lame to remember
(via the global flags structure) if it had written ID3v2 tags and if so
what file offset they finished at and then write the Xing/VBR frame there.

This would mean it would work for a write-only file but would not work if
whoever calls liblame didn't write the returned data to the start of a
file.


----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=100290&aid=1636267&group_id=290