SQL 3.3 xmalloc overflow example - why?

Question

The following example can be found in many places on the internet:

The following code excerpt from OpenSSH 3.3 demonstrates a classic case of integer overflow: (bad code) Example Language: C

nresp = packet_get_int();
if (nresp > 0) {
  response = xmalloc(nresp*sizeof(char*));
  for (i = 0; i < nresp; i++) response[i] = packet_get_string(NULL);
}

If nresp has the value 1073741824 and sizeof(char*) has its typical value of 4, then the result of the operation nresp*sizeof(char*) overflows, and the argument to xmalloc() will be 0. Most malloc() implementations will happily allocate a 0-byte buffer, causing the subsequent loop iterations to overflow the heap buffer response.

If nresp is an int and sizeof() returns a size_t, which in x86_64 is essentially unsigned long, the result should be unsigned long. Is the problem with the above that the argument for xmalloc is an int? Or does the example assume IA32? If not, what's the problem?

Answer 1

If nresp is an int and sizeof() returns a size_t, which in x86_64 is essentially unsigned long, the result should be unsigned long.

True, yet the product can overflow when INT_MAX > ULONG_MAX/sizeof(char*). (e.g. int and unsigned long,size_t both 32-bit.)

---

Even when nresp > 0 and sizeof(char*) is a smallish value, computations like nresp*sizeof(char*) risk overflow given it is an int * size_t.

C does not specify size_t to be wider than int, although size_t is some unsigned type. size_t may only have 1 more bit than int and overflow can occurs when sizeof(char*) is greater than 2.
(On select machine, SIZE_MAX <= INT_MAX is possible.)

Robust code prevents/detects potential overflow without making undue assumptions about the width of size_t and int.

Simple to add a range test

int nresp = packet_get_int();  
if (nresp > 0) {
    response = (unsigned) nresp > SIZE_MAX/sizeof(char*) ? NULL : xmalloc(nresp*sizeof(char*));
    if (response == NULL) {
      ; // TBD code to handle allocation failure.
    } else {
      for (i = 0; i < nresp; i++) response[i] = packet_get_string(NULL);
    }
  }
}

On machines where SIZE_MAX/sizeof(char*) > UINT_MAX, I'd expect response = (unsigned) nresp > SIZE_MAX/sizeof(char*) ? NULL : xmalloc(nresp*sizeof(char*)); to optimize to response = xmalloc(nresp*sizeof(char*));

---

IMO, better as response = (unsigned) nresp > SIZE_MAX/sizeof(char*) ? NULL : xmalloc(sizeof response[0] * (unsigned) nresp);