Use of long long type outside of HAVE_LONGLONG ifdef #478

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

Projects

None yet

3 participants

@gkinseyhpw

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
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
Member
bagder commented Oct 8, 2015

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

@gkinseyhpw

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

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
Member
bagder commented Oct 8, 2015

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

@gkinseyhpw

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

@bagder bagder added a commit that closed this issue Oct 8, 2015
@bagder bagder ntlm: get rid of unconditional use of long long
... since some compilers don't have it and instead use other types, such
as __int64.

Reported by: gkinseyhpw
Closes #478
8256b44
@bagder bagder closed this in 8256b44 Oct 8, 2015
@bagder
Member
bagder commented Oct 8, 2015

thanks for the report, merged fix now!

@jay
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
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
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
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 Oct 9, 2015
@jgsogo jgsogo added a commit to jgsogo/curl that referenced this issue Oct 19, 2015
@bagder @jgsogo bagder + jgsogo ntlm: get rid of unconditional use of long long
... since some compilers don't have it and instead use other types, such
as __int64.

Reported by: gkinseyhpw
Closes #478
c16f704
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment