From: TP Diffenbach <rockbox_at_diffenbach.org>
Date: Mon, 17 Mar 2003 01:39:29 -0500

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!
Received on 2003-03-17