mirrors/nodejs
mirrors/nodejs
0
0
Fork 0
mirror of https://github.com/nodejs/node.git synced 2024-12-01 16:10:02 +01:00
Code Issues Releases Activity

process: fix two overflow cases in SourceMap VLQ decoding

Browse Source 
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

PR-URL: https://github.com/nodejs/node/pull/31490
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
This commit is contained in:
Justin Ridgewell 2020-01-24 02:49:41 -05:00 committed by Rich Trott
parent 59cba9a5c2
commit 0214b90308
2 changed files with 55 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
lib/internal/source_map/source_map.js
View File

@ -303,8 +303,19 @@ 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 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);
}
/**

42
test/parallel/test-source-map-api.js
View File

@ -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]);
}
}