From 36da106b33b65a56f21fe6e315bdc7552cafe0dd Mon Sep 17 00:00:00 2001 From: biril Date: Tue, 15 Nov 2016 22:49:37 +0000 Subject: [PATCH] Fix invalid-index bug in ID3v2 IPLS frame parsing Don't assume that final string in involved people list is null terminated. --- lib/id3v2.js | 29 ++++++++++++++++----------- test/spec/id3v2-ipls.spec.js | 39 +++++++++++++++++++++++++++++++----- 2 files changed, 51 insertions(+), 17 deletions(-) diff --git a/lib/id3v2.js b/lib/id3v2.js index e075ff3..d5017f5 100644 --- a/lib/id3v2.js +++ b/lib/id3v2.js @@ -287,22 +287,22 @@ return { ownerIdentifier: ownerIdentifier, identifier: identifier }; }; - // Read the content of a + // Read the content of an // [involved people list frame](http://id3.org/id3v2.3.0#Involved_people_list). Contains // names of those involved - those contributing to the audio file - and how they were - // involved. The body simply contains a terminated string with the involvement directly - // followed by a terminated string with the involvee followed by a new involvement and so - // on. In the current implementation however, the frame's content is parsed as a - // collection of strings without attaching special meaning.There may only be one "IPLS" - // frame in each tag + // involved. The body simply contains the first 'involvement' as a terminated string, directly + // followed by the first 'involvee' as a terminated string, followed by a second terminated + // involvement string and so on. However, in the current implementation the frame's content is + // parsed as a collection of strings without any semantics attached. There may only be one + // "IPLS" frame in each tag // // * Encoding: a single octet where 0 = ISO-8859-1, 1 = UCS-2 - // * People list strings: a series of strings, e.g. string 00 (00) string 00 (00) ... + // * People list strings: a series of strings, e.g. string 00 (00) string 00 (00) .. readFrameContent.IPLS = function (view, offset, length) { // The content to be returned var content = { encoding: view.getUint8(offset), values: [] }; - // Encoding and content beginning (people list) + // Encoding and content beginning (people list - specifically, first 'involvement' string) var enc = content.encoding === 0 ? "iso" : "ucs"; var offsetBeg = offset + 1; @@ -310,10 +310,15 @@ var offsetNextStrTrm; while (offsetBeg < offset + length) { - offsetNextStrTrm = lib.locateStrTrm[enc](view, offsetBeg, - length - (offsetBeg - offset)); - content.values.push(lib.readStr[enc](view, offsetBeg, - offsetNextStrTrm - offsetBeg)); + // We expect all strings within the people list to be null terminated .. + offsetNextStrTrm = lib.locateStrTrm[enc](view, offsetBeg, length - (offsetBeg - offset)); + + // .. except _perhaps_ the last one. In this case fix the offset at the frame's end + if (offsetNextStrTrm === -1) { + offsetNextStrTrm = offset + length; + } + + content.values.push(lib.readStr[enc](view, offsetBeg, offsetNextStrTrm - offsetBeg)); offsetBeg = offsetNextStrTrm + (enc === "ucs" ? 2 : 1); } diff --git a/test/spec/id3v2-ipls.spec.js b/test/spec/id3v2-ipls.spec.js index 3d1043f..49077f2 100644 --- a/test/spec/id3v2-ipls.spec.js +++ b/test/spec/id3v2-ipls.spec.js @@ -7,11 +7,11 @@ // Read the content of an // [involved people list frame](http://id3.org/id3v2.3.0#Involved_people_list). Contains // names of those involved - those contributing to the audio file - and how they were -// involved. The body simply contains a terminated string with the involvement directly -// followed by a terminated string with the involvee followed by a new involvement and so -// on. In the current implementation however, the frame's content is parsed as a -// collection of strings without attaching special meaning.There may only be one "IPLS" -// frame in each tag +// involved. The body simply contains the first 'involvement' as a terminated string, directly +// followed by the first 'involvee' as a terminated string, followed by a second terminated +// involvement string and so on. However, in the current implementation the frame's content is +// parsed as a collection of strings without any semantics attached. There may only be one +// "IPLS" frame in each tag // // * Encoding: a single octet where 0 = ISO-8859-1, 1 = UCS-2 // * People list strings: a series of strings, e.g. string 00 (00) string 00 (00) ... @@ -46,6 +46,20 @@ describe("ID3v2.3 parser, reading IPLS frame", () => { expect(frame.content.values[1]).toBe("second"); }); + it("should read IPLS frame with content: 0first0second", () => { + const frameView = util.buildFrameView({ + id: "IPLS", + content: [0, "first", 0, "second"], + offset: frameOffset + }); + + const frame = parser.readId3v2TagFrame(frameView, frameOffset, frameView.byteLength); + + expect(frame.content.encoding).toBe(0); + expect(frame.content.values[0]).toBe("first"); + expect(frame.content.values[1]).toBe("second"); + }); + it("should read IPLS frame with content: 0first0second0third0", () => { const frameView = util.buildFrameView({ id: "IPLS", @@ -60,4 +74,19 @@ describe("ID3v2.3 parser, reading IPLS frame", () => { expect(frame.content.values[1]).toBe("second"); expect(frame.content.values[2]).toBe("third"); }); + + it("should read IPLS frame with content: 0first0second0third", () => { + const frameView = util.buildFrameView({ + id: "IPLS", + content: [0, "first", 0, "second", 0, "third"], + offset: frameOffset + }); + + const frame = parser.readId3v2TagFrame(frameView, frameOffset, frameView.byteLength); + + expect(frame.content.encoding).toBe(0); + expect(frame.content.values[0]).toBe("first"); + expect(frame.content.values[1]).toBe("second"); + expect(frame.content.values[2]).toBe("third"); + }); });