Fix invalid-index bug in ID3v2 IPLS frame parsing

Don't assume that final string in involved people list is null terminated.
biril · Nov 15, 2016 · 36da106 · 36da106
1 parent b5613d6
commit 36da106
Show file tree

Hide file tree

Showing 2 changed files with 51 additions and 17 deletions.
diff --git a/lib/id3v2.js b/lib/id3v2.js
@@ -287,33 +287,38 @@
         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;
 
         // Index of null-terminator found within people list (seperates involvement / involvee)
         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
@@ -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");
+    });
 });