From ec033946a9adfb62fde16294022f4fee94906185 Mon Sep 17 00:00:00 2001 From: Justin Ridgewell Date: Fri, 24 Jan 2020 02:49:41 -0500 Subject: [PATCH 1/3] process: fix two overflow cases in SourceMap VLQ decoding These both have to do with extremely large numbers, so it's unlikely to cause a problem in practice. Still, correctness. First, encoding `-2147483648` in VLQ returns the value `"B"`. When decoding, we get the value `1` after reading the base64. We then check if the first bit is set (it is) to see if we should negate it, then we shift all bits right once. Now, `value` will be `0` and `negate` will be `true`. So, we'd return `-0`. Which is a bug! `-0` isn't `-2147483648`, and we've broken a round trip. Second, encoding any number with the 31st bit set, we'd return the opposite sign. Let's use `1073741824`. Encoding, we get `"ggggggC"`. When decoding, we get the value `-2147483648` after reading the base64. Notice, it's already negative (the 32nd bit is set, because the 31st was set and we shifted everything left once). We'd then check the first bit (it's not) and shift right. But we used `>>`, which does not shift the sign bit. We actually wanted `>>>`, which will. Because of that bug, we get back `-1073741824` instead of the positive `1073741824`. It's even worse if the 32nd and 31st bits are set, `-1610612736` becomes `536870912` after a round trip. I recently fixed the same two bugs in Closure Compiler: https://github.com/google/closure-compiler/commit/584418eb --- lib/internal/source_map/source_map.js | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/lib/internal/source_map/source_map.js b/lib/internal/source_map/source_map.js index 32fe43ac8f68cb..f9025feef68396 100644 --- a/lib/internal/source_map/source_map.js +++ b/lib/internal/source_map/source_map.js @@ -303,8 +303,20 @@ function decodeVLQ(stringCharIterator) { // Fix the sign. const negative = result & 1; - result >>= 1; - return negative ? -result : result; + // Use unsigned right shift, so that the 32nd bit is properly shifted to the + // 31st, and the 32nd becomes unset. + result >>>= 1; + if (!negative) { + return result; + } + + // We need to OR 0x80000000 here to ensure the 32nd bit (the sign bit in a + // 32bit int) is always set for negative numbers. If `result` were 1, + // (meaning `negate` is true and all other bits were zeros), `result` would + // now be 0. But -0 doesn't flip the 32nd bit as intended. All other numbers + // will successfully set the 32nd bit without issue, so doing this is a noop + // for them. + return -result | 0x80000000; } /** From 78e7088fc1e2c20578e17d199d2a0ad2155a36ad Mon Sep 17 00:00:00 2001 From: Justin Ridgewell Date: Fri, 24 Jan 2020 21:05:37 -0500 Subject: [PATCH 2/3] process: add tests for SourceMap's decodeVLQ --- test/parallel/test-source-map-api.js | 42 ++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/test/parallel/test-source-map-api.js b/test/parallel/test-source-map-api.js index e7704cf45cf68e..2bfbc08809e9a1 100644 --- a/test/parallel/test-source-map-api.js +++ b/test/parallel/test-source-map-api.js @@ -82,3 +82,45 @@ const { readFileSync } = require('fs'); assert.strictEqual(payload.sources[0], sourceMap.payload.sources[0]); assert.notStrictEqual(payload.sources, sourceMap.payload.sources); } + +// Test various known decodings to ensure decodeVLQ works correctly. +{ + function makeMinimalMap(column) { + return { + sources: ['test.js'], + // Mapping from the 0th line, 0th column of the output file to the 0th + // source file, 0th line, ${column}th column. + mappings: `AAA${column}`, + }; + } + const knownDecodings = { + 'A': 0, + 'B': -2147483648, + 'C': 1, + 'D': -1, + 'E': 2, + 'F': -2, + + // 2^31 - 1, maximum values + '+/////D': 2147483647, + '8/////D': 2147483646, + '6/////D': 2147483645, + '4/////D': 2147483644, + '2/////D': 2147483643, + '0/////D': 2147483642, + + // -2^31 + 1, minimum values + '//////D': -2147483647, + '9/////D': -2147483646, + '7/////D': -2147483645, + '5/////D': -2147483644, + '3/////D': -2147483643, + '1/////D': -2147483642, + }; + + for (const column in knownDecodings) { + const sourceMap = new SourceMap(makeMinimalMap(column)); + const { originalColumn } = sourceMap.findEntry(0, 0); + assert.strictEqual(originalColumn, knownDecodings[column]); + } +} From 9d83dbd12aa41764b7f7a63e6a710f29c49bd311 Mon Sep 17 00:00:00 2001 From: Justin Ridgewell Date: Fri, 24 Jan 2020 21:14:33 -0500 Subject: [PATCH 3/3] process: avoid 0x80000000 --- lib/internal/source_map/source_map.js | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/lib/internal/source_map/source_map.js b/lib/internal/source_map/source_map.js index f9025feef68396..d66230ec37f312 100644 --- a/lib/internal/source_map/source_map.js +++ b/lib/internal/source_map/source_map.js @@ -310,13 +310,12 @@ function decodeVLQ(stringCharIterator) { return result; } - // We need to OR 0x80000000 here to ensure the 32nd bit (the sign bit in a - // 32bit int) is always set for negative numbers. If `result` were 1, - // (meaning `negate` is true and all other bits were zeros), `result` would - // now be 0. But -0 doesn't flip the 32nd bit as intended. All other numbers - // will successfully set the 32nd bit without issue, so doing this is a noop - // for them. - return -result | 0x80000000; + // We need to OR here to ensure the 32nd bit (the sign bit in an Int32) is + // always set for negative numbers. If `result` were 1, (meaning `negate` is + // true and all other bits were zeros), `result` would now be 0. But -0 + // doesn't flip the 32nd bit as intended. All other numbers will successfully + // set the 32nd bit without issue, so doing this is a noop for them. + return -result | (1 << 31); } /**