Skip to content

tfilestream truncate/read bug - MP3 stripping fix#914

Merged
sbooth merged 1 commit into
taglib:masterfrom
whatdoineed2do:mp3-strip-APE-ID3v1-bugfix
Jul 25, 2019
Merged

tfilestream truncate/read bug - MP3 stripping fix#914
sbooth merged 1 commit into
taglib:masterfrom
whatdoineed2do:mp3-strip-APE-ID3v1-bugfix

Conversation

@whatdoineed2do

@whatdoineed2do whatdoineed2do commented Jul 21, 2019

Copy link
Copy Markdown
Contributor

Closes #913.

The fread(3C) function potentially buffers the input stream and needs fflush following a ftruncate(2) being called; ie

FILE* f = fopen(...);
ftruncate(fileno(f), 10);
// fflush(f);  -- this is needed here
fseek(f, 10, SEEK_SET);  // seek to where we've truncated the file
fread(....);   // this will read data that has been truncated but buffered

These functions are used by code by tfilestream for some file operations, in particular the stripping of MP3 tags.

The missing fflush causes a problem with MP3 stripping of APE and ID3v1 tags - these tags reside at the end of the file and in this order if both exist.

Stripping functionality for APE tags seek to the end of the tag (based on APELocation and APEOriginalSize) and performs a blind fread() until it reaches EOF, assuming it to be the ID3v1 tag.

Striipping functionality for ID3v1 tag simply performs a ftruncate() from the start of the tag position (ID3v1Location).

The bug can be observed by:

    f.strip(ID3v1);
    f.strip(APE);  // the ID3v1 will be untouched/not stripped

This means current master executes:

  • truncate file to ID3v1 pos
  • seek to end of APE tag pos and fread() til end of file; the data returned here is potentially buffered (ie reads the ID3v1 tag that was just truncated/stripped)
  • rewrite all the read data (ie the stripped ID3v1) at the APE tag position

This commit performs a simple fflush() following the ftruncate() to discard any buffered data to ensure no phantom data being returned.

The POSIX defintion for fflush() defines the behavior for input streams, whilst the latest draft C2x standard leaves it undefined.

Tested on Linux/Fedora 26.

whatdoineed2do pushed a commit to whatdoineed2do/taglib that referenced this pull request Jul 21, 2019
@whatdoineed2do whatdoineed2do mentioned this pull request Jul 22, 2019
@whatdoineed2do whatdoineed2do force-pushed the mp3-strip-APE-ID3v1-bugfix branch from 4812449 to 520d1e9 Compare July 22, 2019 09:55
@sbooth sbooth self-assigned this Jul 22, 2019
@sbooth

sbooth commented Jul 22, 2019

Copy link
Copy Markdown
Contributor

Thank you for this fix.

Out of curiosity if fflush() is called before ftruncate() instead of after do you see a difference in behavior?

@whatdoineed2do

whatdoineed2do commented Jul 22, 2019

Copy link
Copy Markdown
Contributor Author

Calling fflush()before the ftruncate() also works to fix this bug.

Here's a test prog to demo the issue: a.out /tmp/data 0 for no fflush, change 0 to 1 to observe the fix.

#include <sys/stat.h>
#include <sys/types.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/types.h>
#include <string.h>
#include <stdio.h>
#include <errno.h>



int main ( int argc, char *argv[])
{
    char buf[64];

    if (argc != 3) {
	printf("args: <tmpfile> <0 or 1 to call fflush or not>");
	return -1;
    }

    int fd;

    if ((fd = open (argv[1], O_TRUNC | O_CREAT | O_WRONLY, 0600)) < 0) {
	printf ("failed to create/trunc file '%s' - %s\n", argv[1], strerror (errno));
	return -1;
    }


    const char *data = "0123456789abcdef";

    write (fd, data, strlen (data));
    fsync (fd);
    close (fd);
    fd = -1;

    FILE *f = fopen (argv[1], "rb+");

    if (f == NULL) {
	printf ("failed to open data file\n");
	return -1;
    }

    printf ("data file = '%s'\ntruncating at pos=10, remain = '%s'\n", data, &data[10]);
    fread (buf, 1, 8, f);  // prime the internal stream buffer

    fseek (f, 10, SEEK_SET);
    ftruncate (fileno (f), 10);
    if (*argv[2] == '1') {
	printf ("   flush FILE, will see following read contain NO data \n");
	fflush (f);
    }
    else {
	printf ("NO flush FILE, will see following read contain data\n");
    }

    int r = fread (buf, 1, sizeof (buf), f);
    buf[r] = '\0';
    printf ("fread %ld - '%s'\n", r, buf);
    fclose (f);

    return 0;
}

whatdoineed2do pushed a commit to whatdoineed2do/taglib that referenced this pull request Jul 22, 2019
@sbooth

sbooth commented Jul 24, 2019

Copy link
Copy Markdown
Contributor

Since fflush() before ftruncate() works correctly would you mind to amend the commit with the change? Without combing through the uses of tfilestream I am not sure if this could happen in TagLib, but I think it's possible that a pending fwrite()could have buffered unwritten changes that ftruncate() might alter. Switching the order of the calls will eliminate that possibility.

…ffered data

to avoid presenting phantom data being 'read'/presented to caller via a fread()

Current bug due to the buffered data can be seen in stripping mp3s of tags

    f.strip(ID3v1);
    f.strip(APE);

The ID3v1 tag sits at the end of file (strip calls ftruncate()) and the APE
strip performs a readFile() that would return the stream buffered/truncatd data
and reinsert
whatdoineed2do pushed a commit to whatdoineed2do/taglib that referenced this pull request Jul 24, 2019
@whatdoineed2do whatdoineed2do force-pushed the mp3-strip-APE-ID3v1-bugfix branch from 520d1e9 to f5fb3ee Compare July 24, 2019 19:34
@whatdoineed2do

Copy link
Copy Markdown
Contributor Author

It makes sense. I've made the change and pushed.

@sbooth sbooth merged commit 79bc9cc into taglib:master Jul 25, 2019
whatdoineed2do pushed a commit to whatdoineed2do/taglib that referenced this pull request Jul 25, 2019
@whatdoineed2do whatdoineed2do deleted the mp3-strip-APE-ID3v1-bugfix branch October 5, 2019 12:04
whatdoineed2do pushed a commit to whatdoineed2do/taglib that referenced this pull request Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MP3 strip bug

2 participants