Skip to content

Commit beb3478

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 d02c29f commit beb3478

File tree

4 files changed

+70
-10
lines changed

4 files changed

+70
-10
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: 36 additions & 5 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
//
@@ -363,12 +363,23 @@ function makeSetValue(ptr, pos, value, type, noNeedFirst, ignore, align, sep) {
363363
assert(typeof align === 'undefined', 'makeSetValue no longer supports align parameter');
364364
assert(typeof noNeedFirst === 'undefined', 'makeSetValue no longer supports noNeedFirst parameter');
365365
assert(typeof sep === 'undefined', 'makeSetValue no longer supports sep parameter');
366+
367+
var rtn = makeSetValueImpl(ptr, pos, value, type);
368+
if (ASSERTIONS == 2 && (type.startsWith('i') || type.startsWith('u'))) {
369+
const width = getBitWidth(type);
370+
const assertion = `check${width}(${value})`;
371+
rtn += ';' + assertion
372+
}
373+
return rtn;
374+
}
375+
376+
function makeSetValueImpl(ptr, pos, value, type) {
366377
if (type == 'i64' && !WASM_BIGINT) {
367378
// If we lack either BigInt we must fall back to an reading a pair of I32
368379
// values.
369380
return '(tempI64 = [' + splitI64(value) + '], ' +
370-
makeSetValue(ptr, pos, 'tempI64[0]', 'i32') + ',' +
371-
makeSetValue(ptr, getFastValue(pos, '+', getNativeTypeSize('i32')), 'tempI64[1]', 'i32') + ')';
381+
makeSetValueImpl(ptr, pos, 'tempI64[0]', 'i32') + ',' +
382+
makeSetValueImpl(ptr, getFastValue(pos, '+', getNativeTypeSize('i32')), 'tempI64[1]', 'i32') + ')';
372383
}
373384

374385
const offset = calcFastOffset(ptr, pos);
@@ -381,7 +392,8 @@ function makeSetValue(ptr, pos, value, type, noNeedFirst, ignore, align, sep) {
381392
if (slab == 'HEAPU64' || slab == 'HEAP64') {
382393
value = `BigInt(${value})`;
383394
}
384-
return slab + '[' + getHeapOffset(offset, type) + '] = ' + value;
395+
var rtn = slab + '[' + getHeapOffset(offset, type) + '] = ' + value;
396+
return rtn;
385397
}
386398

387399
function makeHEAPView(which, start, end) {
@@ -448,6 +460,25 @@ function calcFastOffset(ptr, pos) {
448460
return getFastValue(ptr, '+', pos);
449461
}
450462

463+
function getBitWidth(type) {
464+
if (isPointerType(type)) {
465+
type = POINTER_TYPE;
466+
}
467+
switch (type) {
468+
case 'i1': return 1;
469+
case 'i8': // fallthrough
470+
case 'u8': return 8;
471+
case 'i16': // fallthrough
472+
case 'u16': return 16;
473+
case 'i32': // fallthrough
474+
case 'u32': return 32
475+
case 'i53': // fallthrough
476+
case 'u53': return 53
477+
case 'i64': // fallthrough
478+
case 'u64': return 64;
479+
}
480+
}
481+
451482
function getHeapForType(type) {
452483
assert(type);
453484
if (isPointerType(type)) {

src/runtime_debug.js

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,30 @@ 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+
function checkInt(value, bits, max) {
108+
assert(Number.isInteger(Number(value)), "attempt to write non-integer (" + value + ") into integer heap");
109+
assert(value <= max, "value (" + value + ") too large to write as " + bits +"-bit value");
110+
}
111+
112+
var check1 = (value) => checkInt(value, 1, 1);
113+
var check8 = (value) => checkInt(value, 8, MAX_UINT8);
114+
var check16 = (value) => checkInt(value, 16, MAX_UINT16);
115+
var check32 = (value) => checkInt(value, 32, MAX_UINT32);
116+
var check53 = (value) => checkInt(value, 53, MAX_UINT53);
117+
var check64 = (value) => checkInt(value, 64, MAX_UINT64);
118+
119+
#endif // ASSERTIONS == 2
120+
121+
#endif // ASSERTIONS
99122

100123
#if RUNTIME_DEBUG
101124
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)