Skip to content

Commit

Permalink
Add debug range checks for makeSetValue
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
sbc100 committed Apr 28, 2023
1 parent 9ffc857 commit 0af6c79
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 9 deletions.
2 changes: 1 addition & 1 deletion src/library.js
Original file line number Diff line number Diff line change
Expand Up @@ -1899,7 +1899,7 @@ mergeInto(LibraryManager.library, {
{{{ makeSetValue('ai', C_STRUCTS.addrinfo.ai_family, 'family', 'i32') }}};
{{{ makeSetValue('ai', C_STRUCTS.addrinfo.ai_socktype, 'type', 'i32') }}};
{{{ makeSetValue('ai', C_STRUCTS.addrinfo.ai_protocol, 'proto', 'i32') }}};
{{{ makeSetValue('ai', C_STRUCTS.addrinfo.ai_canonname, 'canon', 'i32') }}};
{{{ makeSetValue('ai', C_STRUCTS.addrinfo.ai_canonname, 'canon', '*') }}};
{{{ makeSetValue('ai', C_STRUCTS.addrinfo.ai_addr, 'sa', '*') }}};
if (family === {{{ cDefs.AF_INET6 }}}) {
{{{ makeSetValue('ai', C_STRUCTS.addrinfo.ai_addrlen, C_STRUCTS.sockaddr_in6.__size__, 'i32') }}};
Expand Down
24 changes: 20 additions & 4 deletions src/parseTools.js
Original file line number Diff line number Diff line change
Expand Up @@ -205,13 +205,13 @@ function makeInlineCalculation(expression, value, tempVar) {

// Splits a number (an integer in a double, possibly > 32 bits) into an i64
// value, represented by a low and high i32 pair.
// Will suffer from rounding.
// Will suffer from rounding and truncation.
function splitI64(value) {
// general idea:
//
// $1$0 = ~~$d >>> 0;
// $1$1 = Math.abs($d) >= 1 ? (
// $d > 0 ? Math.floor(($d)/ 4294967296.0) >>> 0,
// $d > 0 ? Math.floor(($d)/ 4294967296.0) >>> 0
// : Math.ceil(Math.min(-4294967296.0, $d - $1$0)/ 4294967296.0)
// ) : 0;
//
Expand Down Expand Up @@ -357,12 +357,22 @@ function makeGetValue(ptr, pos, type, noNeedFirst, unsigned, ignore, align) {
* @return {string} JS code for performing the memory set operation
*/
function makeSetValue(ptr, pos, value, type) {
var rtn = makeSetValueImpl(ptr, pos, value, type);
if (ASSERTIONS == 2 && (type.startsWith('i') || type.startsWith('u'))) {
const width = getBitWidth(type);
const assertion = `checkInt${width}(${value})`;
rtn += ';' + assertion
}
return rtn;
}

function makeSetValueImpl(ptr, pos, value, type) {
if (type == 'i64' && !WASM_BIGINT) {
// If we lack BigInt support we must fall back to an reading a pair of I32
// values.
return '(tempI64 = [' + splitI64(value) + '], ' +
makeSetValue(ptr, pos, 'tempI64[0]', 'i32') + ',' +
makeSetValue(ptr, getFastValue(pos, '+', getNativeTypeSize('i32')), 'tempI64[1]', 'i32') + ')';
makeSetValueImpl(ptr, pos, 'tempI64[0]', 'i32') + ',' +
makeSetValueImpl(ptr, getFastValue(pos, '+', getNativeTypeSize('i32')), 'tempI64[1]', 'i32') + ')';
}

const offset = calcFastOffset(ptr, pos);
Expand Down Expand Up @@ -442,6 +452,12 @@ function calcFastOffset(ptr, pos) {
return getFastValue(ptr, '+', pos);
}

function getBitWidth(type) {
if (type == 'i53' || type == 'u53')
return 53;
return getNativeTypeSize(type) * 8;
}

function getHeapForType(type) {
assert(type);
if (isPointerType(type)) {
Expand Down
32 changes: 31 additions & 1 deletion src/runtime_debug.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,37 @@ function unexportedRuntimeSymbol(sym) {
});
}
}
#endif

#if ASSERTIONS == 2

var MAX_UINT8 = (2 ** 8) - 1;
var MAX_UINT16 = (2 ** 16) - 1;
var MAX_UINT32 = (2 ** 32) - 1;
var MAX_UINT53 = (2 ** 53) - 1;
var MAX_UINT64 = (2 ** 64) - 1;

var MIN_INT8 = - (2 ** ( 8 - 1)) + 1;
var MIN_INT16 = - (2 ** (16 - 1)) + 1;
var MIN_INT32 = - (2 ** (32 - 1)) + 1;
var MIN_INT53 = - (2 ** (53 - 1)) + 1;
var MIN_INT64 = - (2 ** (64 - 1)) + 1;

function checkInt(value, bits, min, max) {
assert(Number.isInteger(Number(value)), "attempt to write non-integer (" + value + ") into integer heap");
assert(value <= max, "value (" + value + ") too large to write as " + bits +"-bit value");
assert(value >= min, "value (" + value + ") too small to write as " + bits +"-bit value");
}

var checkInt1 = (value) => checkInt(value, 1, 1);
var checkInt8 = (value) => checkInt(value, 8, MIN_INT8, MAX_UINT8);
var checkInt16 = (value) => checkInt(value, 16, MIN_INT16, MAX_UINT16);
var checkInt32 = (value) => checkInt(value, 32, MIN_INT32, MAX_UINT32);
var checkInt53 = (value) => checkInt(value, 53, MIN_INT53, MAX_UINT53);
var checkInt64 = (value) => checkInt(value, 64, MIN_INT64, MAX_UINT64);

#endif // ASSERTIONS == 2

#endif // ASSERTIONS

#if RUNTIME_DEBUG
var runtimeDebug = true; // Switch to false at runtime to disable logging at the right times
Expand Down
12 changes: 9 additions & 3 deletions test/test_other.py
Original file line number Diff line number Diff line change
Expand Up @@ -12892,7 +12892,13 @@ def test_no_cfi(self):

@also_with_wasm_bigint
def test_parseTools(self):
self.do_other_test('test_parseTools.c', emcc_args=['--js-library', test_file('other/test_parseTools.js')])
self.emcc_args += ['--js-library', test_file('other/test_parseTools.js')]
self.do_other_test('test_parseTools.c')

# If we run ths same test with -sASSERTIONS=2 we expect it to fail because it
# involves writing numbers that are exceed the side of the type.
expected = 'Aborted(Assertion failed: value (316059037807746200000) too large to write as 64-bit value)'
self.do_runf(test_file('other/test_parseTools.c'), expected, emcc_args=['-sASSERTIONS=2'], assert_returncode=NON_ZERO)

def test_lto_atexit(self):
self.emcc_args.append('-flto')
Expand Down Expand Up @@ -13200,8 +13206,8 @@ def test_parseTools_legacy(self):
create_file('lib.js', '''
mergeInto(LibraryManager.library, {
foo: function() {
return {{{ Runtime.POINTER_SIZE }}};
}
return {{{ Runtime.POINTER_SIZE }}};
}
});
''')
self.set_setting('DEFAULT_LIBRARY_FUNCS_TO_INCLUDE', 'foo')
Expand Down

0 comments on commit 0af6c79

Please sign in to comment.