Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplified _libssh2_check_length #350

Merged
merged 2 commits into from Apr 5, 2019
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

Next

Simplified _libssh2_check_length

misc.c : _libssh2_check_length()

Removed cast and one-lined check. 

Credit : Yuriy M. Kaminskiy
  • Loading branch information...
willco007 committed Apr 3, 2019
commit 5929e269990fcf923fc5371b78a63b39c9fa7048
@@ -811,10 +811,7 @@ int _libssh2_get_bignum_bytes(struct string_buf *buf, unsigned char **outbuf)

int _libssh2_check_length(struct string_buf *buf, size_t len)
{
if(len > buf->len)
return 0;

return ((int)(buf->dataptr - buf->data) <= (int)(buf->len - len)) ? 1 : 0;
return (len <= (size_t)((buf->data + buf->len) - buf->dataptr));

This comment has been minimized.

Copy link
@kdudka

kdudka Apr 4, 2019

Contributor

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.

This comment has been minimized.

Copy link
@willco007

willco007 Apr 4, 2019

Author Member

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.

This comment has been minimized.

Copy link
@bagder

bagder Apr 4, 2019

Member

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

This comment has been minimized.

Copy link
@willco007

willco007 Apr 4, 2019

Author Member

Looks good to me. I'll go ahead and make the change to @bagder's last suggestion.

}

/* Wrappers */
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.