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

Use of long long type outside of HAVE_LONGLONG ifdef #478

Closed
gkinseyhpw opened this issue Oct 8, 2015 · 12 comments
Closed

Use of long long type outside of HAVE_LONGLONG ifdef #478

gkinseyhpw opened this issue Oct 8, 2015 · 12 comments
Assignees

Comments

@gkinseyhpw
Copy link

In lib/curl_ntlm_core.c there is a usage of the "long long" type outside of the ifdef HAVE_LONGLONG which means the code doesn't compile on platforms without "long long".

The following patch fixes it.

Index: curl_ntlm_core.c

--- curl_ntlm_core.c (revision 2295)
+++ curl_ntlm_core.c (revision 2296)
@@ -678,7 +678,11 @@
tw = 11644473600ULL * 10000000ULL;
else
#endif
+#if defined(HAVE_LONGLONG)
tw = ((long long)time(NULL) + 11644473600ULL) * 10000000ULL;
+#else

  • tw = ((__int64)time(NULL) + 11644473600) * 10000000;
    +#endif

/* Calculate the response len */
len = NTLM_HMAC_MD5_LEN + NTLMv2_BLOB_LEN;

@bagder
Copy link
Member

bagder commented Oct 8, 2015

Thanks, but this patch then assumes we have __int64 if we don't have long long and that too is a bad assumption!

@bagder
Copy link
Member

bagder commented Oct 8, 2015

Hm, but one we do elsewhere in that code already...

@gkinseyhpw
Copy link
Author

Yes, I assumed it was safe to use since that is the alternative type it uses when HAVE_LONGLONG is not set a few lines up.

@bagder
Copy link
Member

bagder commented Oct 8, 2015

how about this instead?

diff --git a/lib/curl_ntlm_core.c b/lib/curl_ntlm_core.c
index b72a8dc..756802d 100644
--- a/lib/curl_ntlm_core.c
+++ b/lib/curl_ntlm_core.c
@@ -662,25 +662,22 @@ CURLcode Curl_ntlm_core_mk_ntlmv2_resp(unsigned char *ntlmv2hash,
 */

   unsigned int len = 0;
   unsigned char *ptr = NULL;
   unsigned char hmac_output[NTLM_HMAC_MD5_LEN];
-#if defined(HAVE_LONGLONG)
-  long long tw;
-#else
-  __int64 tw;
-#endif
+  curl_off_t tw;
+
   CURLcode result = CURLE_OK;

   /* Calculate the timestamp */
 #ifdef DEBUGBUILD
   char *force_timestamp = getenv("CURL_FORCETIME");
   if(force_timestamp)
-    tw = 11644473600ULL * 10000000ULL;
+    tw = (curl_off_t)11644473600 * 10000000ULL;
   else
 #endif
-  tw = ((long long)time(NULL) + 11644473600ULL) * 10000000ULL;
+    tw = ((curl_off_t)time(NULL) + 11644473600) * 10000000ULL;

   /* Calculate the response len */
   len = NTLM_HMAC_MD5_LEN + NTLMv2_BLOB_LEN;

   /* Allocate the response */

@bagder bagder self-assigned this Oct 8, 2015
@gkinseyhpw
Copy link
Author

The type change looks good (I'll do a test compile in a minute to confirm), but the ULL on the literal needs to be removed since VC6 doesn't understand that either.

@bagder
Copy link
Member

bagder commented Oct 8, 2015

Oops, you're right. I meant to do that on all of them.

@gkinseyhpw
Copy link
Author

Patch tested (with ULL removed), compiles fine. Thanks for the extremely fast response.

@bagder bagder closed this as completed in 8256b44 Oct 8, 2015
@bagder
Copy link
Member

bagder commented Oct 8, 2015

thanks for the report, merged fix now!

@jay
Copy link
Member

jay commented Oct 9, 2015

As noted in the function, the timestamp is supposed to be a "LE, 64-bit signed value representing the number of tenths of a microsecond since January 1, 1601." Using curl_off_t if it may be < 64 bits when ntlm is enabled doesn't seem correct since it can't represent a timestamp (epoch conversion to what is basically a FILETIME requires more than 32 bits to represent the resulting value in all cases).

We either need a type guaranteed at least 64 bits or we have to do something that allows us to represent one like split each of those numbers in two 32bit dwords (this is actually what FILETIME does) and then do the multiplication on them in pieces. I don't really see a reason to do all that at the moment (who else has this problem?) however It may be better to be more explicit about what is required:

diff --git a/lib/curl_ntlm_core.c b/lib/curl_ntlm_core.c
index b72a8dc..f10d514 100644
--- a/lib/curl_ntlm_core.c
+++ b/lib/curl_ntlm_core.c
@@ -665,20 +665,21 @@ CURLcode Curl_ntlm_core_mk_ntlmv2_resp(unsigned char *ntlm
   unsigned char *ptr = NULL;
   unsigned char hmac_output[NTLM_HMAC_MD5_LEN];
 #if defined(HAVE_LONGLONG)
-  long long tw;
+  typedef long long twtype;
 #else
-  __int64 tw;
+  typedef __int64 twtype;
 #endif
+  twtype tw;
   CURLcode result = CURLE_OK;

   /* Calculate the timestamp */
 #ifdef DEBUGBUILD
   char *force_timestamp = getenv("CURL_FORCETIME");
   if(force_timestamp)
-    tw = 11644473600ULL * 10000000ULL;
+    tw = 11644473600 * 10000000;
   else
 #endif
-  tw = ((long long)time(NULL) + 11644473600ULL) * 10000000ULL;
+  tw = ((twtype)time(NULL) + 11644473600) * 10000000;

   /* Calculate the response len */
   len = NTLM_HMAC_MD5_LEN + NTLMv2_BLOB_LEN;

Also as you know C89 does not say anything about type representation in numeric literals for decimal past unsigned long so even if a compiler supports __int64 it seems to me the numeric literal representation is implementation dependent. In the case of VC6 though I did an empirical test and it appears to work so if it's something large like like 11644473600 without a suffix it goes in __int64.

@bagder
Copy link
Member

bagder commented Oct 9, 2015

Alternatively, as I already merged the change to turn that into using curl_off_t, we add another check that makes sure that it really is a 64 bit data type as it could make it a bit clearer to the developer.

Of course we could also rewrite the logic to not requite 64bit logic, but I'm not sure its worth the effort unless we know we actually have users who'd care.

#if CURL_SIZEOF_CURL_OFF_T < 8
#error "needs 64bit support to build"
#endif

@bagder bagder reopened this Oct 9, 2015
@jay
Copy link
Member

jay commented Oct 9, 2015

we add another check that makes sure that it really is a 64 bit data type as it could make it a bit clearer to the developer

👍

@bagder
Copy link
Member

bagder commented Oct 9, 2015

Grrr, typo in the commit message for 13ddb9e now refers to the wrong issue. Merged anyway just now!

@bagder bagder closed this as completed Oct 9, 2015
jgsogo pushed a commit to jgsogo/curl that referenced this issue Oct 19, 2015
... since some compilers don't have it and instead use other types, such
as __int64.

Reported by: gkinseyhpw
Closes curl#478
@lock lock bot locked as resolved and limited conversation to collaborators May 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

3 participants