writeTag can clobber MP3 stream due to bad padding calculation

[Bart Massey]

From: Roman Cheplyaka <email address hidden>

Consider the 'else' code path in hWriteTag: when the new tag is smaller
than the old tag. The padding is correctly updated to account for the
difference, but that does not seem to translate into an updated header
size. Thus the declared header size (which comes form the new, small
tag) is smaller than the actual size.

[Bart Massey (bart-massey)]

I am having a hard time verifying the bug, but have rewritten a bunch of WriteTag.hs to make it more transparent, which may help. I am now working on adding custom padding size support to WriteTag.hs. When I have this right, I will add some tests to the test suite to check all the cases of padding. Apologies if this takes a while. If you have a good test case, please do send it to me.

[Bart Massey (bart-massey)]

From: Roman Cheplyaka <email address hidden>

Here's a different code path reproducing, I believe, the same bug.

Take an mp3 file without any tag at all (example attached).

Then run this program on it:

module Main where

import ID3
import ID3.Simple
import System.Environment

main :: IO ()
main = do
  [n, path] <- getArgs
  let
    tag = initID3Tag
      [ setTitle (replicate (read n) 'x') ]
  writeTag path tag

Like this:

 cp test0.mp3 test.mp3 && idiii-bug-exe 2 test.mp3

You can then check in a hex editor that the declared header size is 0x0D
(corresponding to the bare tag without padding), while there is a
4096-byte padding present.

As I said, in both cases the issue seems to be that updating the padding
does not affect the declared header size.

[Bart Massey (bart-massey)]

Oh, I see! Thanks much for the code: it makes it clear to me what is going on and gives me something to test my patches against. Will fix as promptly as able. Like I say, the big problem is that we have no regression tests for writeTag at all right now: that really needs to be fixed before I muck around too much with its code. :-)