Skip to content

Commit

Permalink
Pointer overflow check
Browse files Browse the repository at this point in the history
When checking for integer overflow, you may often write tests like `p + i < p`. This works fine if `p` and `i` are unsigned integers, since any overflow in the addition will cause the value to simply "wrap around." However, using this pattern when `p` is a pointer is problematic because pointer overflow has undefined behavior according to the C and C++ standards. If the addition overflows and has an undefined result, the comparison will likewise be undefined; it may produce an unintended result, or may be deleted entirely by an optimizing compiler.

In this case I removed the pointer overflow check completely and replaced it with a check of the actual pointer size, as everything outside would be a segmentation fault anyway.
  • Loading branch information
Kraemii committed May 31, 2023
1 parent 4f9ccd9 commit 1f540e4
Showing 1 changed file with 6 additions and 6 deletions.
12 changes: 6 additions & 6 deletions samba/auth/ntlmssp/ntlmssp_parse.c
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ BOOL msrpc_parse(TALLOC_CTX *mem_ctx, const DATA_BLOB *blob,
/* if odd length and unicode */
return False;
}
if (blob->data + ptr < (uint8_t *)ptr || blob->data + ptr < blob->data)
if (ptr > blob->length)
return False;

if (0 < len1) {
Expand Down Expand Up @@ -263,7 +263,7 @@ BOOL msrpc_parse(TALLOC_CTX *mem_ctx, const DATA_BLOB *blob,
return False;
}

if (blob->data + ptr < (uint8_t *)ptr || blob->data + ptr < blob->data)
if (ptr > blob->length)
return False;

if (0 < len1) {
Expand Down Expand Up @@ -293,7 +293,7 @@ BOOL msrpc_parse(TALLOC_CTX *mem_ctx, const DATA_BLOB *blob,
return False;
}

if (blob->data + ptr < (uint8_t *)ptr || blob->data + ptr < blob->data)
if (head_ofs > blob->length)
return False;

*b = data_blob_talloc(mem_ctx, blob->data + ptr, len1);
Expand All @@ -304,7 +304,7 @@ BOOL msrpc_parse(TALLOC_CTX *mem_ctx, const DATA_BLOB *blob,
len1 = va_arg(ap, uint_t);
/* make sure its in the right format - be strict */
NEED_DATA(len1);
if (blob->data + head_ofs < (uint8_t *)head_ofs || blob->data + head_ofs < blob->data)
if (head_ofs > blob->length)
return False;

*b = data_blob_talloc(mem_ctx, blob->data + head_ofs, len1);
Expand All @@ -318,8 +318,8 @@ BOOL msrpc_parse(TALLOC_CTX *mem_ctx, const DATA_BLOB *blob,
case 'C':
s = va_arg(ap, char *);

if (blob->data + head_ofs < (uint8_t *)head_ofs || blob->data + head_ofs < blob->data)
return False;
if (head_ofs > blob->length)
return False;

head_ofs += pull_string(p, blob->data+head_ofs, sizeof(p),
blob->length - head_ofs,
Expand Down

0 comments on commit 1f540e4

Please sign in to comment.