Harden low-level VM bytecode validation and constant sizing

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
 -----------------------------

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;
 

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,

numexpr/numexpr_object.cpp

@@ -8,6 +8,7 @@
 **********************************************************************/
 
 #include "module.hpp"
+#include <limits.h>
 #include <structmember.h>
 
 #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[] = {