Original bug ID: 7492
Reporter: @murmour
Status: acknowledged (set by @dra27 on 2017-02-23T09:20:07Z)
Resolution: open
Priority: normal
Severity: minor
Version: 4.06.0 +dev/beta1/beta2/rc1
Category: runtime system and C interface
Monitored by: @gasche @yallop @yakobowski

Bug description

There are numerous places inside the runtime where integer multiplication or addition is performed without an overflow check. In many of those situations, overflow is practically impossible because the operands are known to be small enough (e.g. a small integer is multiplied by a sizeof(type)), but sometimes it's far from obvious, and so exists the danger of triggering undefined behaviour.

The said above applies to stdlib C stubs as well.

One common source of potential errors involves memory allocation. The malignant pattern is "caml_stat_alloc(x * y)", which can be hardened by making it into "caml_stat_calloc(x, y)". Calloc is supposed to perform an overflow check, and does so in many implementations, although it's unclear whether it is required to do so by the standard. So far, calloc is only used in a couple of places inside the whole runtime. Perhaps it would be beneficial to use it in place of all occurrences of "caml_stat_alloc(x * y)"?

Recently, as part of #71, the implementation of caml_stat_calloc was changed so that it no longer makes use of the system-provided calloc. The new implementation performs no overflow checks at all. If calloc is to be used more commonly in the runtime, caml_stat_calloc must be augmented with a proper overflow check.

Checking for overflow in a portable way is non-trivial in C. Fortunately, we already have a battle-tested routine for safe multiplication -- caml_ba_multov in bigarray_stubs.c. I did quite a bit of research, and AFAIK this approach to overflow detection strikes the best balance of portability and performance. The only faster solution involves non-standard type uint128_t that is not available in Visual Studio and older versions of GCC.

How about moving caml_ba_multov into misc.h (as a differently named #define) so that it could be used elsewhere (e.g. in caml_stat_calloc)?

More danger comes from so called tainted sources -- that is, external sources of untrusted data. Examples of tainted sources are: user input, compiler parameters, environment variables, arguments to C stubs.

For example, these primitives from otherlibs/graph take integers from the user and perform multiplication without an overflow check, which may lead to dangerous consequences:
caml_gr_new_image
caml_gr_fill_arc_nat
caml_gr_draw_arc_nat
caml_gr_create_image

If there were a standard overflow check routine (in misc.h or elsewhere), it could be used to harden such functions.