Rockbox.org home
release
dev builds
extras
themes manual
wiki
device status forums
mailing lists
IRC bugs
patches
dev guide



Rockbox mail archive

Subject: id3.c: reading id3 tags, is this a bug?
From: TP Diffenbach (rockbox_at_diffenbach.org)
Date: 2003-03-17


In id3.c, at line 388, the folowing code finds the artist name from the mp3 tag:

        /* Check for certain frame headers */
        if (!entry->artist &&
            (!strncmp(header, "TPE1", strlen("TPE1")) ||
             !strncmp(header, "TP1", strlen("TP1")))) {
            bytesread = read(fd, buffer + bufferpos, framelen);
            entry->artist = buffer + bufferpos;
            unicode_munge(&entry->artist, &bytesread);
            entry->artist[bytesread + 1] = '\0';
            bufferpos += bytesread + 2;
            size -= bytesread;
        }

unicode_munge is called at line 394 to adjust the string if it's in UNICODE; it's called with the string's length as a pointer to int, so that the passed in length can also be adjusted by unicode_munge.

The (possibly adjusted) length is then used to nul terminate the read string, and to set bufferpos, the position at which the next string will be written. So far so good. All of these values refer to the copy of the string we are saving.

However, size refers to the size of the frame being read from, in the mp3's id3 tag. It seems to me that even if unicode_munge is adjusting the size of the copy of the string that we are saving, the size remaining on disk in the mp3's id3 tag remains the same. If this is the case, size should be set /before/ bytesread is (possibly) adjusted by unicode_munge:

        /* Check for certain frame headers */
        if (!entry->artist &&
            (!strncmp(header, "TPE1", strlen("TPE1")) ||
             !strncmp(header, "TP1", strlen("TP1")))) {
            bytesread = read(fd, buffer + bufferpos, framelen);
            size -= bytesread;
            entry->artist = buffer + bufferpos;
            unicode_munge(&entry->artist, &bytesread);
            entry->artist[bytesread + 1] = '\0';
            bufferpos += bytesread + 2;
        }

But perhaps I don't understand clearly enough what the variable size is being used for.

Also, it seems that the call to read should be checked for error or EOF:

        /* Check for certain frame headers */
        if (!entry->artist &&
            (!strncmp(header, "TPE1", strlen("TPE1")) ||
             !strncmp(header, "TP1", strlen("TP1")))) {
            bytesread = read(fd, buffer + bufferpos, framelen);
            if( bytesread > 0 ) {
              size -= bytesread;
              entry->artist = buffer + bufferpos;
              unicode_munge(&entry->artist, &bytesread);
              entry->artist[bytesread + 1] = '\0';
              bufferpos += bytesread + 2;
            }
        }

Thanks.--
Archos FM needs a Rockbox!



Page was last modified "Jan 10 2012" The Rockbox Crew
aaa