From 3ae345a690a3bb3c0a6c98b73ec2de3a8ce07c20 Mon Sep 17 00:00:00 2001 From: mengw Date: Mon, 3 Aug 2026 15:43:14 +0200 Subject: [PATCH] Harden low-level VM bytecode validation and constant sizing check_program() validated register indexes and most signature characters but missed several ways for crafted bytecode to reach unsafe VM paths: the ('i','l') type exemption let an 8-byte opcode write a 4-byte register; any opcode could store into a read-only input or constant register; a non-final instruction could write a reduction program's single-element output; OP_COPY_SS could copy from a register other than the one the output dtype was sized from, or into a string temporary whose item size is 0; the guard for an operand held in an extended instruction word was unreachable, so it was read out of bounds; and an embedded NUL byte in the signature silently shortened fullsig, desynchronising it from the register map. Validation also ran after NumExpr_init had already installed the program on the object, so a rejected program stayed runnable, and re-initialising an object freed register buffers that a concurrent run() was still using. Validate before installing, accept only the first successful __init__, and refuse to run an object whose __init__ never completed. Size replicated constant storage with checked, non-truncating arithmetic instead of 32-bit int that could wrap before the constants were copied in. Finally, stop dereferencing zero-length string operands in stringcmp() and give an empty operand the correct ordering, so "a < b''" matches NumPy. Adds regression tests for each case and registers them in suite(). --- RELEASE_NOTES.rst | 15 ++++ numexpr/interpreter.cpp | 106 ++++++++++++++++++++---- numexpr/interpreter.hpp | 3 +- numexpr/numexpr_object.cpp | 64 ++++++++++++--- numexpr/tests/test_numexpr.py | 148 ++++++++++++++++++++++++++++++++++ 5 files changed, 309 insertions(+), 27 deletions(-) diff --git a/RELEASE_NOTES.rst b/RELEASE_NOTES.rst index ee49c2a2..d77b032a 100644 --- a/RELEASE_NOTES.rst +++ b/RELEASE_NOTES.rst @@ -14,6 +14,21 @@ Changes from 2.14.2 to 2.14.3 by the sanitizer raise ``ValueError``; and unknown functions raise ``TypeError``. Sanitization can still be explicitly disabled with ``sanitize=False`` or ``NUMEXPR_SANITIZE=0``. +* Hardened low-level VM bytecode validation against mismatched register widths, + writes to read-only registers, truncated extended instructions, unsafe string + copies, string temporaries, a non-final instruction writing the output buffer + of a reduction program, register signatures desynchronised by an embedded NUL + byte, and integer overflow while sizing constant storage. Validation now runs + *before* the program is installed on the ``NumExpr`` object, and ``run()`` + refuses to execute an object whose ``__init__`` never completed. +* ``numexpr.interpreter.NumExpr`` objects are now single-initialisation: calling + ``__init__`` again on a built object raises ``RuntimeError`` instead of + freeing the register buffers that a concurrent ``run()`` may still be using. + A *failed* ``__init__`` installs nothing and can be retried. +* Fixed the ordering of string comparisons against an empty string: an empty + operand now sorts before a non-empty one, so ``a < b''`` and ``a <= b''`` + agree with NumPy, and two empty operands compare equal without reading + uninitialised memory. Changes from 2.14.1 to 2.14.2 ----------------------------- diff --git a/numexpr/interpreter.cpp b/numexpr/interpreter.cpp index f3777214..dadfb9d9 100644 --- a/numexpr/interpreter.cpp +++ b/numexpr/interpreter.cpp @@ -393,15 +393,20 @@ get_reduction_axis(PyObject* program) { +/* Validate a program against the register layout it will run on. This takes + the raw pieces rather than a NumExprObject so that NumExpr_init can call it + *before* installing anything into the object. */ int -check_program(NumExprObject *self) +check_program(PyObject *program_object, PyObject *fullsig_object, + PyObject *signature_object, int n_constants, int n_temps) { unsigned char *program; - Py_ssize_t prog_len, n_buffers, n_inputs; + Py_ssize_t prog_len, n_buffers, n_inputs, first_temp, reg; int pc, arg, argloc, argno, sig; char *fullsig, *signature; + bool is_reduction; - if (PyBytes_AsStringAndSize(self->program, (char **)&program, + if (PyBytes_AsStringAndSize(program_object, (char **)&program, &prog_len) < 0) { PyErr_Format(PyExc_RuntimeError, "invalid program: can't read program"); return -1; @@ -410,12 +415,16 @@ check_program(NumExprObject *self) PyErr_Format(PyExc_RuntimeError, "invalid program: prog_len mod 4 != 0"); return -1; } - if (PyBytes_AsStringAndSize(self->fullsig, (char **)&fullsig, + if (prog_len == 0) { + PyErr_SetString(PyExc_RuntimeError, "invalid program: program is empty"); + return -1; + } + if (PyBytes_AsStringAndSize(fullsig_object, (char **)&fullsig, &n_buffers) < 0) { PyErr_Format(PyExc_RuntimeError, "invalid program: can't read fullsig"); return -1; } - if (PyBytes_AsStringAndSize(self->signature, (char **)&signature, + if (PyBytes_AsStringAndSize(signature_object, (char **)&signature, &n_inputs) < 0) { PyErr_Format(PyExc_RuntimeError, "invalid program: can't read signature"); return -1; @@ -424,6 +433,26 @@ check_program(NumExprObject *self) PyErr_Format(PyExc_RuntimeError, "invalid program: too many buffers"); return -1; } + /* fullsig is built with PyBytes_FromFormat("%c%s%s%s", ...), whose %s stops + at the first NUL byte. An embedded NUL in signature or tempsig would + shorten fullsig without shortening mem[]/memsizes[], so every register + index past the NUL would then be described by the wrong signature + character. Reject the whole class by checking the layout invariant. */ + if (n_buffers != 1 + n_inputs + n_constants + n_temps) { + PyErr_Format(PyExc_RuntimeError, + "invalid program: fullsig describes %i buffers but the register map " + "has %i (1 output + %i inputs + %i constants + %i temporaries); " + "signature and tempsig must not contain NUL bytes", + (int)n_buffers, 1 + (int)n_inputs + n_constants + n_temps, + (int)n_inputs, n_constants, n_temps); + return -1; + } + first_temp = 1 + n_inputs + n_constants; + /* A reduction program accumulates into the output register, which + NumExpr_run allocates as a *single* element for a full reduction. Only + the final reduction instruction may write it -- an ordinary opcode + targeting register 0 writes a whole BLOCK_SIZE1 block past its end. */ + is_reduction = program[prog_len-4] > OP_REDUCTION; for (pc = 0; pc < prog_len; pc += 4) { unsigned int op = program[pc]; if (op == OP_NOOP) { @@ -445,11 +474,13 @@ check_program(NumExprObject *self) argloc = pc+argno+1; } if (argno >= 3) { - if (pc + 1 >= prog_len) { - PyErr_Format(PyExc_RuntimeError, "invalid program: double opcode (%c) at end (%i)", pc, sig); + argloc = pc+argno+2; + if (argloc >= prog_len) { + PyErr_Format(PyExc_RuntimeError, + "invalid program: truncated double instruction for opcode %u at %i", + op, pc); return -1; } - argloc = pc+argno+2; } arg = program[argloc]; @@ -525,16 +556,49 @@ check_program(NumExprObject *self) PyErr_Format(PyExc_RuntimeError, "invalid program: internal checker error processing %i", argloc); return -1; } - /* The next is to avoid problems with the ('i','l') duality, - specially in 64-bit platforms */ - } else if (((sig == 'l') && (fullsig[arg] == 'i')) || - ((sig == 'i') && (fullsig[arg] == 'l'))) { - ; } else if (sig != fullsig[arg]) { PyErr_Format(PyExc_RuntimeError, - "invalid : opcode signature doesn't match buffer (%c vs %c) at %i", sig, fullsig[arg], argloc); + "invalid program: opcode signature doesn't match buffer (%c vs %c) at %i", sig, fullsig[arg], argloc); + return -1; + } + if (sig != 'n' && argno == 0 && arg != 0 && arg < first_temp) { + PyErr_Format(PyExc_RuntimeError, + "invalid program: destination buffer is read-only (%i) at %i", + arg, argloc); + return -1; + } + if (sig != 'n' && argno == 0 && arg == 0 && is_reduction && + pc != prog_len-4) { + PyErr_Format(PyExc_RuntimeError, + "invalid program: only the final reduction instruction may " + "write the output buffer (at %i)", pc); return -1; } + if (op == OP_COPY_SS) { + if (argno == 0 && arg != 0) { + PyErr_SetString(PyExc_RuntimeError, + "invalid program: copy_ss destination must be the output buffer"); + return -1; + } + if (argno == 1 && arg != 1) { + PyErr_SetString(PyExc_RuntimeError, + "invalid program: copy_ss source must be buffer 1"); + return -1; + } + } + } + } + /* String registers have a zero item size (size_from_char('s') == 0), so a + string temporary is a zero-length allocation that the string comparison + opcodes would still read from. No opcode can write one either, since + OP_COPY_SS is the only string-producing opcode and its destination is + the output buffer. */ + for (reg = first_temp; reg < n_buffers; reg++) { + if (fullsig[reg] == 's') { + PyErr_Format(PyExc_RuntimeError, + "invalid program: string temporaries are not supported (%i)", + (int)reg); + return -1; } } return 0; @@ -576,8 +640,13 @@ stringcmp(const char *s1, const char *s2, npy_intp maxlen1, npy_intp maxlen2) // First check if some of the operands is the empty string and if so, // just check that the first char of the other is the NULL one. // Fixes #121 + // Two empty operands compare equal without dereferencing either pointer: + // a zero-sized register holds no readable byte at all. + if (maxlen1 == 0 && maxlen2 == 0) return 0; if (maxlen2 == 0) return *s1 != null; - if (maxlen1 == 0) return *s2 != null; + /* An empty s1 sorts *before* a non-empty s2, so the sign must be negative + here -- returning +1 made "a < b''" and "a <= b''" disagree with NumPy. */ + if (maxlen1 == 0) return -(*s2 != null); maxlen = (maxlen1 > maxlen2) ? maxlen1 : maxlen2; for (nextpos = 1; nextpos <= maxlen; nextpos++) { @@ -1076,6 +1145,13 @@ NumExpr_run(NumExprObject *self, PyObject *args, PyObject *kwds) // Don't force serial mode by default gs.force_serial = 0; + // A NumExprObject that never completed __init__() (e.g. NumExpr.__new__()) + // carries an empty program, which last_opcode() would read out of bounds. + if (PyBytes_GET_SIZE(self->program) < 4) { + PyErr_SetString(PyExc_RuntimeError, "invalid program: program is empty"); + return NULL; + } + // Check whether there's a reduction as the final step is_reduction = last_opcode(self->program) > OP_REDUCTION; diff --git a/numexpr/interpreter.hpp b/numexpr/interpreter.hpp index 3ec09bb7..b25f1f8d 100644 --- a/numexpr/interpreter.hpp +++ b/numexpr/interpreter.hpp @@ -128,7 +128,8 @@ extern thread_data th_params; PyObject *NumExpr_run(NumExprObject *self, PyObject *args, PyObject *kwds); char get_return_sig(PyObject* program); -int check_program(NumExprObject *self); +int check_program(PyObject *program_object, PyObject *fullsig_object, + PyObject *signature_object, int n_constants, int n_temps); int get_temps_space(const vm_params& params, char **mem, size_t block_size); void free_temps_space(const vm_params& params, char **mem); int vm_engine_iter_task(NpyIter *iter, npy_intp *memsteps, diff --git a/numexpr/numexpr_object.cpp b/numexpr/numexpr_object.cpp index b6e2f9c1..0a9cb445 100644 --- a/numexpr/numexpr_object.cpp +++ b/numexpr/numexpr_object.cpp @@ -8,6 +8,7 @@ **********************************************************************/ #include "module.hpp" +#include #include #include "numexpr_config.hpp" @@ -86,20 +87,31 @@ NumExpr_new(PyTypeObject *type, PyObject *args, PyObject *kwds) static int NumExpr_init(NumExprObject *self, PyObject *args, PyObject *kwds) { - int i, j, mem_offset; + int i, j; int n_inputs, n_constants, n_temps; PyObject *signature = NULL, *tempsig = NULL, *constsig = NULL; PyObject *fullsig = NULL, *program = NULL, *constants = NULL; PyObject *input_names = NULL, *o_constants = NULL; - int *itemsizes = NULL; + Py_ssize_t *itemsizes = NULL; char **mem = NULL, *rawmem = NULL; npy_intp *memsteps; npy_intp *memsizes; + Py_ssize_t mem_offset, program_size; int rawmemsize; static char *kwlist[] = {CHARP("signature"), CHARP("tempsig"), CHARP("program"), CHARP("constants"), CHARP("input_names"), NULL}; + /* A NumExpr object is immutable once built (all its members are READONLY), + and run() hands self->mem to worker threads while the GIL is released. + Re-initialising it would PyMem_Del() those buffers underneath a running + interpreter, so only the first successful __init__ is accepted. */ + if (self->mem != NULL) { + PyErr_SetString(PyExc_RuntimeError, + "NumExpr objects cannot be re-initialised"); + return -1; + } + if (!PyArg_ParseTupleAndKeywords(args, kwds, "SSS|OO", kwlist, &signature, &tempsig, @@ -108,6 +120,13 @@ NumExpr_init(NumExprObject *self, PyObject *args, PyObject *kwds) return -1; } + program_size = PyBytes_GET_SIZE(program); + if (program_size < 4 || program_size % 4 != 0) { + PyErr_SetString(PyExc_RuntimeError, + "invalid program: expected at least one complete instruction"); + return -1; + } + n_inputs = (int)PyBytes_Size(signature); n_temps = (int)PyBytes_Size(tempsig); @@ -123,7 +142,7 @@ NumExpr_init(NumExprObject *self, PyObject *args, PyObject *kwds) Py_DECREF(constants); return -1; } - if (!(itemsizes = PyMem_New(int, n_constants))) { + if (!(itemsizes = PyMem_New(Py_ssize_t, n_constants))) { Py_DECREF(constants); Py_DECREF(constsig); return -1; @@ -173,7 +192,7 @@ NumExpr_init(NumExprObject *self, PyObject *args, PyObject *kwds) } if (PyBytes_Check(o)) { PyBytes_AS_STRING(constsig)[i] = 's'; - itemsizes[i] = (int)PyBytes_GET_SIZE(o); + itemsizes[i] = PyBytes_GET_SIZE(o); continue; } PyErr_SetString(PyExc_TypeError, "constants must be of type bool/int/long/float/double/complex/bytes"); @@ -209,9 +228,21 @@ NumExpr_init(NumExprObject *self, PyObject *args, PyObject *kwds) /* Compute the size of registers. We leave temps out (will be malloc'ed later on). */ rawmemsize = 0; - for (i = 0; i < n_constants; i++) - rawmemsize += itemsizes[i]; - rawmemsize *= BLOCK_SIZE1; + for (i = 0; i < n_constants; i++) { + /* Keep the allocation within the range supported by the old int + rawmemsize field, but reject overflow instead of wrapping it. */ + if (itemsizes[i] > INT_MAX / BLOCK_SIZE1 || + rawmemsize > INT_MAX - itemsizes[i] * BLOCK_SIZE1) { + PyErr_SetString(PyExc_OverflowError, + "total constant storage is too large"); + Py_DECREF(constants); + Py_DECREF(constsig); + Py_DECREF(fullsig); + PyMem_Del(itemsizes); + return -1; + } + rawmemsize += (int)(itemsizes[i] * BLOCK_SIZE1); + } mem = PyMem_New(char *, 1 + n_inputs + n_constants + n_temps); rawmem = PyMem_New(char, rawmemsize); @@ -238,7 +269,7 @@ NumExpr_init(NumExprObject *self, PyObject *args, PyObject *kwds) mem_offset = 0; for (i = 0; i < n_constants; i++) { char c = PyBytes_AS_STRING(constsig)[i]; - int size = itemsizes[i]; + Py_ssize_t size = itemsizes[i]; mem[i+n_inputs+1] = rawmem + mem_offset; mem_offset += BLOCK_SIZE1 * size; memsteps[i+n_inputs+1] = memsizes[i+n_inputs+1] = size; @@ -286,8 +317,8 @@ NumExpr_init(NumExprObject *self, PyObject *args, PyObject *kwds) } else if (c == 's') { char *smem = (char*)mem[i+n_inputs+1]; char *value = PyBytes_AS_STRING(PyTuple_GET_ITEM(constants, i)); - for (j = 0; j < size*BLOCK_SIZE1; j+=size) { - memcpy(smem + j, value, size); + for (j = 0; j < BLOCK_SIZE1; j++) { + memcpy(smem + j*size, value, size); } } } @@ -317,6 +348,17 @@ NumExpr_init(NumExprObject *self, PyObject *args, PyObject *kwds) return -1; } + /* Validate the program *before* installing it. */ + if (check_program(program, fullsig, signature, n_constants, n_temps) < 0) { + Py_DECREF(constants); + Py_DECREF(constsig); + Py_DECREF(fullsig); + PyMem_Del(mem); + PyMem_Del(rawmem); + PyMem_Del(memsteps); + PyMem_Del(memsizes); + return -1; + } #define REPLACE_OBJ(arg) \ {PyObject *tmp = self->arg; \ @@ -345,7 +387,7 @@ NumExpr_init(NumExprObject *self, PyObject *args, PyObject *kwds) #undef INCREF_REPLACE_OBJ #undef REPLACE_MEM - return check_program(self); + return 0; } static PyMethodDef NumExpr_methods[] = { diff --git a/numexpr/tests/test_numexpr.py b/numexpr/tests/test_numexpr.py index 5fdeebb9..8b575f09 100644 --- a/numexpr/tests/test_numexpr.py +++ b/numexpr/tests/test_numexpr.py @@ -378,6 +378,134 @@ class test_numexpr2(test_numexpr): nthreads = 2 +class test_interpreter_validation(TestCase): + """Malformed bytecode must be rejected before entering the VM.""" + + def test_rejects_integer_width_mismatch(self): + program = bytes([ + 50, 2, 1, 1, # add_lll into an int32 temporary + 47, 0, 1, 0, # copy_ll into the output + ]) + with self.assertRaisesRegex(RuntimeError, "signature doesn't match"): + numexpr.interpreter.NumExpr( + b'l', b'i', program, (), (b'x',) + ) + + def test_rejects_truncated_double_instruction(self): + # where_fbff reads its fourth operand from the following word. + program = bytes([77, 0, 1, 2]) + with self.assertRaisesRegex(RuntimeError, "truncated double instruction"): + numexpr.interpreter.NumExpr( + b'bff', b'', program, (), (b'a', b'b', b'c') + ) + + def test_rejects_incomplete_program(self): + for program in (b'', b'\0'): + with self.assertRaisesRegex(RuntimeError, "complete instruction"): + numexpr.interpreter.NumExpr( + b'', b'', program, (), None + ) + + def test_rejects_copy_ss_from_noncanonical_source(self): + program = bytes([116, 0, 2, 0]) + with self.assertRaisesRegex(RuntimeError, "copy_ss source"): + numexpr.interpreter.NumExpr( + b'', b'', program, (b'a', b'b' * 16), None + ) + + def test_rejects_copy_ss_into_temporary(self): + program = bytes([ + 116, 2, 1, 0, + 116, 0, 1, 0, + ]) + with self.assertRaisesRegex(RuntimeError, "copy_ss destination"): + numexpr.interpreter.NumExpr( + b's', b's', program, (), (b'x',) + ) + + def test_rejects_store_into_input(self): + program = bytes([ + 29, 1, 1, 0, # copy_ii into read-only input register 1 + 29, 0, 1, 0, + ]) + with self.assertRaisesRegex(RuntimeError, "destination buffer is read-only"): + numexpr.interpreter.NumExpr( + b'i', b'', program, (), (b'x',) + ) + + def test_rejects_string_temporary(self): + # A 's' temporary has item size 0, so it is a zero-length allocation + # that eq_bss would still read from. + program = bytes([26, 0, 2, 2]) + with self.assertRaisesRegex(RuntimeError, "string temporaries"): + numexpr.interpreter.NumExpr( + b's', b's', program, (), (b'x',) + ) + + def test_rejects_reinitialisation(self): + # Re-initialising would free the register buffers that a concurrent + # run() has already handed to the worker threads, and it was also a way + # to install a program that never passed validation. + valid = bytes([116, 0, 1, 0]) + nex = numexpr.interpreter.NumExpr(b's', b'', valid, (), (b'x',)) + for program in (valid, bytes([116, 2, 2, 0])): + with self.assertRaisesRegex(RuntimeError, "re-initialised"): + nex.__init__(b'bss', b'', program, (), (b'a', b'b', b'c')) + self.assertEqual(nex.program, valid) + self.assertEqual(nex.signature, b's') + x = array([b'abcdefgh'] * 8, dtype='S8') + assert_array_equal(nex.run(x), x) + + def test_failed_init_can_be_retried(self): + # Nothing is installed by a rejected __init__, so the half-built object + # is still usable for a second, valid attempt. + nex = numexpr.interpreter.NumExpr.__new__(numexpr.interpreter.NumExpr) + with self.assertRaisesRegex(RuntimeError, "read-only"): + nex.__init__(b'i', b'', bytes([29, 1, 1, 0, 29, 0, 1, 0]), (), (b'x',)) + nex.__init__(b'i', b'', bytes([29, 0, 1, 0]), (), (b'x',)) + x = arange(8, dtype='int32') + assert_array_equal(nex.run(x), x) + + def test_rejects_reduction_writing_output_before_the_end(self): + # A full reduction allocates a single-element output, so an ordinary + # opcode targeting register 0 would write a whole block past its end. + program = bytes([ + 88, 0, 1, 1, # mul_ddd into the output buffer + 140, 0, 1, 0, # min_ddn -- makes the output a lone accumulator + ]) + with self.assertRaisesRegex(RuntimeError, "final reduction instruction"): + numexpr.interpreter.NumExpr( + b'd', b'', program, (), (b'x',) + ) + + def test_rejects_nul_in_signature(self): + # PyBytes_FromFormat("%s") truncates at a NUL, which would desynchronise + # fullsig from the register map that mem[]/memsizes[] are indexed with. + program = bytes([26, 0, 2, 2]) + for signature, tempsig in ((b'i\x00', b's'), (b'i\x00i', b'd')): + with self.assertRaisesRegex(RuntimeError, "NUL bytes"): + numexpr.interpreter.NumExpr( + signature, tempsig, program, (), (b'a', b'b') + ) + + def test_uninitialized_object_refuses_to_run(self): + nex = numexpr.interpreter.NumExpr.__new__(numexpr.interpreter.NumExpr) + with self.assertRaisesRegex(RuntimeError, "program is empty"): + nex.run() + + def test_rejects_oversized_constant_storage(self): + constants = ( + b'a' * 2_000_000, + b'b' * 2_000_000, + b'c' * 194_304, + ) + program = bytes([116, 0, 1, 0]) + with self.assertRaisesRegex(OverflowError, "constant storage"): + numexpr.interpreter.NumExpr( + b'', b'', program, constants, None + ) + + class test_evaluate(TestCase): def test_simple(self): a = array([1., 2., 3.]) @@ -1288,6 +1416,24 @@ def test_compare_prefix(self): s1, s2 = b'foo', b'foo\0\0' self.assertTrue(evaluate('s1 == s2')) + def test_compare_empty_string_ordering(self): + # An empty operand sorts before every non-empty one; ordering against + # an empty constant must agree with NumPy. + a = np.array([b'foo', b'', b'bar']) + for expr, expected in ( + ("a < b''", a < b''), + ("a <= b''", a <= b''), + ("a > b''", a > b''), + ("a >= b''", a >= b''), + ("a == b''", a == b''), + ("a != b''", a != b''), + ("b'' < a", b'' < a), + ("b'' <= a", b'' <= a), + ): + assert_array_equal(evaluate(expr), expected, err_msg=expr) + self.assertTrue(evaluate("b'' == b''")) + self.assertFalse(evaluate("b'' < b''")) + # Case for testing selections in fields which are aligned but whose # data length is not an exact multiple of the length of the record. @@ -1639,6 +1785,8 @@ def test_method(self): theSuite.addTest(unittest.defaultTestLoader.loadTestsFromTestCase(test_numexpr)) if 'sparc' not in platform.machine(): theSuite.addTest(unittest.defaultTestLoader.loadTestsFromTestCase(test_numexpr2)) + theSuite.addTest( + unittest.defaultTestLoader.loadTestsFromTestCase(test_interpreter_validation)) theSuite.addTest(unittest.defaultTestLoader.loadTestsFromTestCase(test_evaluate)) # Add the dynamically created TestExpressions to the suite if pytest_available: