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?

Are you stuck wth 32-bit systems? Are they embedded? I'd debate your characterization of "sizeof(char *) has its typical value of 4" — it's 8 on 64-bit systems. And on such systems, sizeof(size_t) == 8 so there isn't arithmetic overflow with the nresp value you quote. A bigger problem is that the code doesn't check that the space was allocated successfully — that's an invitation for disaster. The code should be designed so that values that are to large are eliminated before they cause trouble. However, much code does not take such issues seriously. — Jonathan Leffler
– Jonathan Leffler, Commented Jan 24, 2024 at 20:04
I claim no originality for the quote, and I doubt that the (anonymous) person who wrote "There is no portable code; there is only code that has been ported" in 2002 would claim any originality, either on their own behalf or on their husband's behalf. I've quoted the epithet on SO before. — and I claim no originality either. — Jonathan Leffler
– Jonathan Leffler, Commented Jan 24, 2024 at 20:12
Re “which in x86_64 is a long”: Nowhere in the quoted text does it say the platform is x86_64. You have added an assumption. Quite obviously, the author of the text did not use that assumption. (Also size_t can never be long, as it must be unsigned.) — Eric Postpischil
– Eric Postpischil, Commented Jan 24, 2024 at 20:15
The type of size_t is never long; it is always an unsigned type. Often it is equivalent to unsigned long, which may be 32-bits or 64-bits, depending on the platform. Beware Windows 64 — long is still a 32-bit type, even though pointers are 64-bit types. — Jonathan Leffler
– Jonathan Leffler, Commented Jan 24, 2024 at 20:16

chux · Accepted Answer · 2024-01-24 22:38:38Z

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

edited Jan 24, 2024 at 22:38

answered Jan 24, 2024 at 22:13

chux

158k17 gold badges160 silver badges311 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

HangingParens Over a year ago

OK, so it sounds like the issue has nothing to do with xmalloc(), and if size_t is a 64-bit unsigned long, and if int is 32-bits, then there would be no problem with this code.

chux Over a year ago

@HangingParens Yes, If size_t is 64-bit and ìnt` is 32-bit and if xmalloc()does not return NULL, then no problem, today, with the original code. Yet with simply tests, we can write code that does not depend on those ifs. Your call.

Collectives™ on Stack Overflow

SQL 3.3 xmalloc overflow example - why?

1 Answer 1

2 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

2 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related