vauth/cleartext: fix integer overflow check #2408

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@bagder
Member

bagder commented Mar 20, 2018

Make the integer overflow check not rely on the undefined behavior that
a size_t wraps around on overflow.

Detected by lgtm.com

vauth/cleartext: fix integer overflow check
Make the integer overflow check not rely on the undefined behavior that
a size_t wraps around on overflow.

Detected by lgtm.com

@bagder bagder closed this in c136657 Mar 20, 2018

@bagder bagder deleted the bagder/cleartext-fix-overflow-check branch Mar 20, 2018

@jay

This comment has been minimized.

Show comment Hide comment
@jay

jay Mar 20, 2018

Member

I use that overflow check pattern all the time, how is that undefined? size_t should always be unsigned unless gcc in early 90s maybe where they didn't follow the standard exactly. I think that checker is too sensitive. What about a username and password length check some small value like 1k instead of some chunk of size_t max

Member

jay commented Mar 20, 2018

I use that overflow check pattern all the time, how is that undefined? size_t should always be unsigned unless gcc in early 90s maybe where they didn't follow the standard exactly. I think that checker is too sensitive. What about a username and password length check some small value like 1k instead of some chunk of size_t max

@bagder

This comment has been minimized.

Show comment Hide comment
@bagder

bagder Mar 20, 2018

Member

Ah yes, I was a bit "blinded" by the warning so I didn't think properly. It actually shouldn't be undefined, no...

What about a username and password length check some small value like 1k instead of some chunk of size_t max

Yeah, I think that would make sense and would actually probably help to detect errors earlier and better...

Member

bagder commented Mar 20, 2018

Ah yes, I was a bit "blinded" by the warning so I didn't think properly. It actually shouldn't be undefined, no...

What about a username and password length check some small value like 1k instead of some chunk of size_t max

Yeah, I think that would make sense and would actually probably help to detect errors earlier and better...

@jay

This comment has been minimized.

Show comment Hide comment
@jay

jay Mar 23, 2018

Member

ok how about this

diff --git a/lib/vauth/cleartext.c b/lib/vauth/cleartext.c
index 5d61ce6..b9a9be0 100644
--- a/lib/vauth/cleartext.c
+++ b/lib/vauth/cleartext.c
@@ -74,7 +74,7 @@ CURLcode Curl_auth_create_plain_message(struct Curl_easy *data,
   plen = strlen(passwdp);
 
   /* Compute binary message length. Check for overflows. */
-  if((ulen > SIZE_T_MAX/2) || (plen > (SIZE_T_MAX/2 - 2)))
+  if(ulen > 1024 || plen > 1024)
     return CURLE_OUT_OF_MEMORY;
   plainlen = 2 * ulen + plen + 2;
 
Member

jay commented Mar 23, 2018

ok how about this

diff --git a/lib/vauth/cleartext.c b/lib/vauth/cleartext.c
index 5d61ce6..b9a9be0 100644
--- a/lib/vauth/cleartext.c
+++ b/lib/vauth/cleartext.c
@@ -74,7 +74,7 @@ CURLcode Curl_auth_create_plain_message(struct Curl_easy *data,
   plen = strlen(passwdp);
 
   /* Compute binary message length. Check for overflows. */
-  if((ulen > SIZE_T_MAX/2) || (plen > (SIZE_T_MAX/2 - 2)))
+  if(ulen > 1024 || plen > 1024)
     return CURLE_OUT_OF_MEMORY;
   plainlen = 2 * ulen + plen + 2;
 
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment