Simplified _libssh2_check_length #350
Conversation
misc.c : _libssh2_check_length() Removed cast and one-lined check. Credit : Yuriy M. Kaminskiy
src/misc.c
Outdated
| return 0; | ||
|
|
||
| return ((int)(buf->dataptr - buf->data) <= (int)(buf->len - len)) ? 1 : 0; | ||
| return (len <= (size_t)((buf->data + buf->len) - buf->dataptr)); |
There was a problem hiding this comment.
This weakens the check in case buf->dataptr is already behind buf->data + buf->len because when ((buf->data + buf->len) - buf->dataptr) is negative, the conversion to size_t turns it into a big positive number.
There was a problem hiding this comment.
It theoretically does, but dataptr should be greater than or equal to data if the API is used correctly (famous last words, I know). Also, in the original case if dataptr was less than data it would be a negative number which would be less than the test and incorrectly return true, which is also bad. Furthermore, the cast to a signed value from unsigned isn't great and could loose precision.
There was a problem hiding this comment.
I think the expression could be split up in several parts to become more readable and then the logic is easier to follow and confirm. Something like:
char *endp = &buf->data[buf->len];
size_t left = endp - buf->dataptr;
return (len <= left);it could even protect against the wrap-around case @kdudka mentioned:
char *endp = &buf->data[buf->len];
size_t left = endp - buf->dataptr;
return ((len <= left) && (left <= buf->len));There was a problem hiding this comment.
Looks good to me. I'll go ahead and make the change to @bagder's last suggestion.
Updated suggested patch to protect against incorrect usage which could cause a wrap-around value to return success.
|
https://blog.semmle.com/libssh2-integer-overflow/ is related to this pull request. |
misc.c : _libssh2_check_length()
Removed cast and one-lined check.
Credit : Yuriy M. Kaminskiy