Skip to content

Commit 129d158

Browse files
committed
Add debug range checks for makeSetValue
Trying to store a value outside the range of the type being stored to can result in unexpected behaviour. For example, storing 256 to a u8 will end up storing 1. Adding these checks discovered real bug in out library code in `getaddrinfo`. I ran the full other and core test suite with these checks enabled at ASSERTIONS=1 before deciding to use ASSERTIONS=2, at least for now.
1 parent 8874899 commit 129d158

File tree

4 files changed

+61
-9
lines changed

4 files changed

+61
-9
lines changed

src/library.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1899,7 +1899,7 @@ mergeInto(LibraryManager.library, {
18991899
{{{ makeSetValue('ai', C_STRUCTS.addrinfo.ai_family, 'family', 'i32') }}};
19001900
{{{ makeSetValue('ai', C_STRUCTS.addrinfo.ai_socktype, 'type', 'i32') }}};
19011901
{{{ makeSetValue('ai', C_STRUCTS.addrinfo.ai_protocol, 'proto', 'i32') }}};
1902-
{{{ makeSetValue('ai', C_STRUCTS.addrinfo.ai_canonname, 'canon', 'i32') }}};
1902+
{{{ makeSetValue('ai', C_STRUCTS.addrinfo.ai_canonname, 'canon', '*') }}};
19031903
{{{ makeSetValue('ai', C_STRUCTS.addrinfo.ai_addr, 'sa', '*') }}};
19041904
if (family === {{{ cDefs.AF_INET6 }}}) {
19051905
{{{ makeSetValue('ai', C_STRUCTS.addrinfo.ai_addrlen, C_STRUCTS.sockaddr_in6.__size__, 'i32') }}};

src/parseTools.js

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -205,13 +205,13 @@ function makeInlineCalculation(expression, value, tempVar) {
205205

206206
// Splits a number (an integer in a double, possibly > 32 bits) into an i64
207207
// value, represented by a low and high i32 pair.
208-
// Will suffer from rounding.
208+
// Will suffer from rounding and truncation.
209209
function splitI64(value) {
210210
// general idea:
211211
//
212212
// $1$0 = ~~$d >>> 0;
213213
// $1$1 = Math.abs($d) >= 1 ? (
214-
// $d > 0 ? Math.floor(($d)/ 4294967296.0) >>> 0,
214+
// $d > 0 ? Math.floor(($d)/ 4294967296.0) >>> 0
215215
// : Math.ceil(Math.min(-4294967296.0, $d - $1$0)/ 4294967296.0)
216216
// ) : 0;
217217
//
@@ -357,12 +357,22 @@ function makeGetValue(ptr, pos, type, noNeedFirst, unsigned, ignore, align) {
357357
* @return {string} JS code for performing the memory set operation
358358
*/
359359
function makeSetValue(ptr, pos, value, type) {
360+
var rtn = makeSetValueImpl(ptr, pos, value, type);
361+
if (ASSERTIONS == 2 && (type.startsWith('i') || type.startsWith('u'))) {
362+
const width = getBitWidth(type);
363+
const assertion = `checkInt${width}(${value})`;
364+
rtn += ';' + assertion
365+
}
366+
return rtn;
367+
}
368+
369+
function makeSetValueImpl(ptr, pos, value, type) {
360370
if (type == 'i64' && !WASM_BIGINT) {
361371
// If we lack BigInt support we must fall back to an reading a pair of I32
362372
// values.
363373
return '(tempI64 = [' + splitI64(value) + '], ' +
364-
makeSetValue(ptr, pos, 'tempI64[0]', 'i32') + ',' +
365-
makeSetValue(ptr, getFastValue(pos, '+', getNativeTypeSize('i32')), 'tempI64[1]', 'i32') + ')';
374+
makeSetValueImpl(ptr, pos, 'tempI64[0]', 'i32') + ',' +
375+
makeSetValueImpl(ptr, getFastValue(pos, '+', getNativeTypeSize('i32')), 'tempI64[1]', 'i32') + ')';
366376
}
367377

368378
const offset = calcFastOffset(ptr, pos);
@@ -442,6 +452,12 @@ function calcFastOffset(ptr, pos) {
442452
return getFastValue(ptr, '+', pos);
443453
}
444454

455+
function getBitWidth(type) {
456+
if (type == 'i53' || type == 'u53')
457+
return 53;
458+
return getNativeTypeSize(type) * 8;
459+
}
460+
445461
function getHeapForType(type) {
446462
assert(type);
447463
if (isPointerType(type)) {

src/runtime_debug.js

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,37 @@ function unexportedRuntimeSymbol(sym) {
9595
});
9696
}
9797
}
98-
#endif
98+
99+
#if ASSERTIONS == 2
100+
101+
var MAX_UINT8 = (2 ** 8) - 1;
102+
var MAX_UINT16 = (2 ** 16) - 1;
103+
var MAX_UINT32 = (2 ** 32) - 1;
104+
var MAX_UINT53 = (2 ** 53) - 1;
105+
var MAX_UINT64 = (2 ** 64) - 1;
106+
107+
var MIN_INT8 = - (2 ** ( 8 - 1)) + 1;
108+
var MIN_INT16 = - (2 ** (16 - 1)) + 1;
109+
var MIN_INT32 = - (2 ** (32 - 1)) + 1;
110+
var MIN_INT53 = - (2 ** (53 - 1)) + 1;
111+
var MIN_INT64 = - (2 ** (64 - 1)) + 1;
112+
113+
function checkInt(value, bits, min, max) {
114+
assert(Number.isInteger(Number(value)), "attempt to write non-integer (" + value + ") into integer heap");
115+
assert(value <= max, "value (" + value + ") too large to write as " + bits +"-bit value");
116+
assert(value >= min, "value (" + value + ") too small to write as " + bits +"-bit value");
117+
}
118+
119+
var checkInt1 = (value) => checkInt(value, 1, 1);
120+
var checkInt8 = (value) => checkInt(value, 8, MIN_INT8, MAX_UINT8);
121+
var checkInt16 = (value) => checkInt(value, 16, MIN_INT16, MAX_UINT16);
122+
var checkInt32 = (value) => checkInt(value, 32, MIN_INT32, MAX_UINT32);
123+
var checkInt53 = (value) => checkInt(value, 53, MIN_INT53, MAX_UINT53);
124+
var checkInt64 = (value) => checkInt(value, 64, MIN_INT64, MAX_UINT64);
125+
126+
#endif // ASSERTIONS == 2
127+
128+
#endif // ASSERTIONS
99129

100130
#if RUNTIME_DEBUG
101131
var runtimeDebug = true; // Switch to false at runtime to disable logging at the right times

test/test_other.py

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12892,7 +12892,13 @@ def test_no_cfi(self):
1289212892

1289312893
@also_with_wasm_bigint
1289412894
def test_parseTools(self):
12895-
self.do_other_test('test_parseTools.c', emcc_args=['--js-library', test_file('other/test_parseTools.js')])
12895+
self.emcc_args += ['--js-library', test_file('other/test_parseTools.js')]
12896+
self.do_other_test('test_parseTools.c')
12897+
12898+
# If we run ths same test with -sASSERTIONS=2 we expect it to fail because it
12899+
# involves writing numbers that are exceed the side of the type.
12900+
expected = 'Aborted(Assertion failed: value (316059037807746200000) too large to write as 64-bit value)'
12901+
self.do_runf(test_file('other/test_parseTools.c'), expected, emcc_args=['-sASSERTIONS=2'], assert_returncode=NON_ZERO)
1289612902

1289712903
def test_lto_atexit(self):
1289812904
self.emcc_args.append('-flto')
@@ -13200,8 +13206,8 @@ def test_parseTools_legacy(self):
1320013206
create_file('lib.js', '''
1320113207
mergeInto(LibraryManager.library, {
1320213208
foo: function() {
13203-
return {{{ Runtime.POINTER_SIZE }}};
13204-
}
13209+
return {{{ Runtime.POINTER_SIZE }}};
13210+
}
1320513211
});
1320613212
''')
1320713213
self.set_setting('DEFAULT_LIBRARY_FUNCS_TO_INCLUDE', 'foo')

0 commit comments

Comments
 (0)