Skip to content

Commit

Permalink
There were two overflow cases hidden in the fromVLQSigned function:
Browse files Browse the repository at this point in the history
1. Integer.MIN_VALUE would return 0, since -0 is just 0. All other negative numbers were fine.
2. Any number with the 31st bit set would be negated, because it used >> instead of >>>. So the 31st bit would be set to the 32nd (sign bit) when encoding, then remain at the 32nd bit when decoding.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=257839681
  • Loading branch information
jridgewell authored and tjgq committed Jul 13, 2019
1 parent 224ba4b commit 584418e
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 2 deletions.
12 changes: 10 additions & 2 deletions src/com/google/debugging/sourcemap/Base64VLQ.java
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,16 @@ private static int toVLQSigned(int value) {
*/
private static int fromVLQSigned(int value) {
boolean negate = (value & 1) == 1;
value = value >> 1;
return negate ? -value : value;
value = value >>> 1;
if (!negate) {
return value;
}
// We need to OR 0x80000000 here to ensure the 32nd bit (the sign bit) is
// always set for negative numbers. If `value` were 1, (meaning `negate` is
// true and all other bits were zeros), `value` would now be 0. -0 is just
// 0, and doesn't flip the 32nd bit as intended. All positive numbers will
// successfully flip the 32nd bit without issue, so it's a noop for them.
return -value | 0x80000000;
}

/**
Expand Down
11 changes: 11 additions & 0 deletions test/com/google/debugging/sourcemap/Base64VLQTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,17 @@ public void testBase64VLQSelectedSignedValues2() {
}
}

@Test
public void testBase64VLQSelectedSignedValues3() {
testValue(Integer.MAX_VALUE);
testValue(Integer.MAX_VALUE - 1);
testValue(Integer.MAX_VALUE - 2);
testValue(0x70000000);
testValue(Integer.MIN_VALUE);
testValue(Integer.MIN_VALUE + 1);
testValue(Integer.MIN_VALUE + 2);
}

static class CharIteratorImpl implements Base64VLQ.CharIterator {
private int current;
private int length;
Expand Down

0 comments on commit 584418e

Please sign in to comment.