Skip to content

Commit

Permalink
Avoid reading the frame unless there's enough data
Browse files Browse the repository at this point in the history
This is a sanity check to avoid issues when the tag ends with padding that is not declared in the extended header.
Fixes #69.
  • Loading branch information
aadsm committed Aug 6, 2017
1 parent 5d71696 commit 752fd52
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 5 deletions.
23 changes: 18 additions & 5 deletions src/ID3v2FrameReader.js
Original file line number Diff line number Diff line change
Expand Up @@ -205,9 +205,12 @@ class ID3v2FrameReader {
tags: ?Array<string>
): TagFrames {
var frames = {};

while (offset < end) {
var frameHeaderSize = this._getFrameHeaderSize(id3header);
// console.log('header', id3header);
while (
// we should be able to read at least the frame header
offset < (end - frameHeaderSize)
) {
var header = this._readFrameHeader(data, offset, id3header);
var frameId = header.id;

Expand Down Expand Up @@ -280,31 +283,41 @@ class ID3v2FrameReader {
return frames;
}

static _getFrameHeaderSize(id3header: TagHeader): number {
var major = id3header.major;

if (major == 2) {
return 6;
} else if (major == 3 || major == 4) {
return 10;
} else {
return 0;
}
}

static _readFrameHeader(
data: MediaFileReader,
offset: number,
id3header: TagHeader
): TagFrameHeader {
var major = id3header.major;
var flags = null;
var frameHeaderSize = this._getFrameHeaderSize(id3header);

switch (major) {
case 2:
var frameId = data.getStringAt(offset, 3);
var frameSize = data.getInteger24At(offset+3, true);
var frameHeaderSize = 6;
break;

case 3:
var frameId = data.getStringAt(offset, 4);
var frameSize = data.getLongAt(offset+4, true);
var frameHeaderSize = 10;
break;

case 4:
var frameId = data.getStringAt(offset, 4);
var frameSize = data.getSynchsafeInteger32At(offset+4);
var frameHeaderSize = 10;
break;
}

Expand Down
24 changes: 24 additions & 0 deletions src/__tests__/ID3v2FrameReader-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -413,4 +413,28 @@ describe("ID3v2FrameReader", function() {
);
expect(tags.MP3e).not.toBeDefined();
});

// Some tags were found to have padding but no extended header declaring the
// padding size (https://github.com/aadsm/jsmediatags/issues/69).
it("should ignore undeclared padding", function() {
var artistName = bin("Lead Artist");
var fileData = [].concat(
bin("TPE1"),
[0x00, 0x00, 0x00, 1 + artistName.length], // size
[0x00, 0x00], // flags
[0x00], // text encoding
artistName, // content
// undeclared padding
[0x00, 0x00]
);
var fileReader = new ArrayFileReader(fileData);
var id3header = {major: 4};

var tags = ID3v2FrameReader.readFrames(
0,
fileData.length,
fileReader,
id3header
);
});
});

0 comments on commit 752fd52

Please sign in to comment.