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

Miscellaneous cast fixes (detected on Windows) #1652

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@dscho
Contributor

dscho commented Jul 7, 2017

On Windows, long is always 32-bit. This is a bit surprising to some code bases, and when compiling cURL with ./configure --disable-shared --enable-debug --enable-maintainer-mode --enable-werror --with-ssl in Git for Windows' SDK (which is a special-purpose version of MSYS2, itself kind of a portable version of Cygwin), these fixes were necessary to complete the build.

Note: I am not quite certain what this does to other platforms, so I am curious to see what Travis spits out.

dscho added some commits Jul 7, 2017

curl_rtmp: fix a compiler warning
The headers of librtmp declare the socket as `int`, and on Windows, that
disagrees with curl_socket_t.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
openssl: fix compiler warning
tv_sec and tv_usec are declared as long on Windows. This is a 32-bit
type, which means that it disagrees with size_t on 64-bit Windows.

Let's just cast the right-hand side to long and shut up the compiler.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@mention-bot

This comment has been minimized.

mention-bot commented Jul 7, 2017

@dscho, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagder, @petrpisaratlascz and @captain-caveman2k to be potential reviewers.

@coveralls

This comment has been minimized.

coveralls commented Jul 7, 2017

Coverage Status

Coverage decreased (-0.02%) to 75.498% when pulling 19686b3 on dscho:cast-fixes into bbc9c6d on curl:master.

@bagder

This comment has been minimized.

Member

bagder commented Jul 7, 2017

The timeval struct is certainly a tricky one to use everywhere without warnings!

POSIX systems have the tv_sec done as time_t. There are known systems with 32 bit time_t and yet 64 bit longs (some AIX version IIRC) and there are systems with 64 bit time_t and 32 bit longs (OpenBSD comes to mind).

This code was recently brought in 1928770 by @dmitrykos. Instead of filling it up with typecasts, I propose that we instead rewrite it to use the timeval struct as a raw buffer and we just scramble that instead. What do you think @dmitrykos?

I mean something in this style:

--- a/lib/vtls/openssl.c
+++ b/lib/vtls/openssl.c
@@ -288,20 +288,19 @@ static CURLcode Curl_ossl_seed(struct Curl_easy *data)
      time */
   do {
     unsigned char randb[64];
     size_t len = sizeof(randb);
     size_t i, i_max;
+    long xor = 0xbea57;
     for(i = 0, i_max = len / sizeof(struct timeval); i < i_max; ++i) {
       struct timeval tv = curlx_tvnow();
-      Curl_wait_ms(1);
-      tv.tv_sec *= i + 1;
-      tv.tv_usec *= i + 2;
-      tv.tv_sec ^= ((curlx_tvnow().tv_sec + curlx_tvnow().tv_usec) *
-                    (i + 3)) << 8;
-      tv.tv_usec ^= ((curlx_tvnow().tv_sec + curlx_tvnow().tv_usec) *
-                     (i + 4)) << 16;
-      memcpy(&randb[i * sizeof(struct timeval)], &tv, sizeof(struct timeval));
+      long *p = (long *)&tv;
+      size_t loop;
+      for(loop=0; loop < sizeof(tv)/sizeof(long); loop++)
+        p[loop] ^= (xor + (long)(loop?p[loop-1]<<3:tv.tv_usec));
+      xor ^= p[0];
+      memcpy(&randb[i * sizeof(struct timeval)], p, sizeof(struct timeval));
     }
     RAND_add(randb, (int)len, (double)len/2);
   } while(!rand_enough());
 
   /* generates a default path for the random seed file */
@dmitrykos

This comment has been minimized.

Contributor

dmitrykos commented Jul 7, 2017

@badger the idea of new implementation looks ok to me but I would add more uncertainty by reverting Curl_wait_ms(1); back into the loop because it adds some more time randomization on a systems with low timer resolution (like Windows) and is not bad idea on other systems too.

Also I would modify p[loop] ^= (xor + (long)(loop?p[loop-1]<<3:tv.tv_usec)); to at least p[loop] ^= (xor + (long)(loop?p[loop-1]<<3:curlx_tvnow().tv_usec));.

In my initial implementation curlx_tvnow() was called 4 times for xoring needs but above changes will make algorithm less predictable too and are ok to my view.

Or another simple fix preserving original logic would be to change declaration from size_t i, i_max; to long i, i_max;.

@bagder

This comment has been minimized.

Member

bagder commented Jul 7, 2017

it adds some more time randomization on a systems with low timer resolution (like Windows)

Windows often has much worse resolution, like 15ms or so, which makes a millisecond sleep not change anything. The simple truth is that when we only have time of day to use as input to fake a random number, the options are very limited...

at least p[loop] ^= (xor + (long)(loop?p[loop-1]<<3:curlx_tvnow().tv_usec));.

Look at the formula again. It only uses the usec if loop is 0, which is exactly once in the loop and it already invokes curlx_tvnow() once per loop so your suggested change actually makes no difference.

@dmitrykos

This comment has been minimized.

Contributor

dmitrykos commented Jul 9, 2017

Curl_wait_ms(1) guarantees that 2nd part of the random byte vector is not created from the same tv values. On Windows with low timer resolution Curl_wait_ms(1) will cause sleep time with the same low resolution and tv will change for sure.

Look at the formula again. It only uses the usec if loop is 0, which is exactly once in the loop and it already invokes curlx_tvnow() once per loop

Yes, sorry. Mistakenly assumed (mixed with previous logic) that tv_usec from the first call has participated in bit twiddling, of course in such situation curlx_tvnow().tv_usec call is not needed.

@dscho

This comment has been minimized.

Contributor

dscho commented Jul 9, 2017

Should I just wait with updating this Pull Request until you got the changes you are discussing into master? (I feel really unqualified to implement them, as it is way over my head what you are talking about...)

@bagder

This comment has been minimized.

Member

bagder commented Jul 10, 2017

@dscho yes, let's agree on this first.

It came to me that it might be a nicer fix anyway to once and for all change curlx_tvnow() to return a custom struct instead of "struct timeval" (struct curlval =) ) that uses our own set types for all platforms so that we can write code easier without warnings everywhere. Like perhaps simply time_t for for the seconds and unsigned int for the microseconds?

I'm on vacation right now so it might take me a few days but unless someone else does it, I might write up a PR for such a change and then the original code @dmitrykos wrote should be possible to build warning-free.

bagder added a commit that referenced this pull request Jul 21, 2017

timeval: introduce curlval as a struct timeval replacement
... to make all libcurl internals able to use the same data types for
the struct members. The timeval struct differs subtly on several
platforms so it makes it cumbersome to use everywhere.

Ref: #1652

bagder added a commit that referenced this pull request Jul 28, 2017

timeval: struct curltime is a struct timeval replacement
... to make all libcurl internals able to use the same data types for
the struct members. The timeval struct differs subtly on several
platforms so it makes it cumbersome to use everywhere.

Ref: #1652
Closes #1693

bagder added a commit that referenced this pull request Jul 28, 2017

curl_rtmp: fix a compiler warning
The headers of librtmp declare the socket as `int`, and on Windows, that
disagrees with curl_socket_t.

Bug: #1652

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@bagder

This comment has been minimized.

Member

bagder commented Jul 28, 2017

I hope I fixed the openssl warnings with #1693 and I cherry-picked the rtmp fix in commit 1cfa4cd. Can we close this now?

@dmitrykos

This comment has been minimized.

Contributor

dmitrykos commented Jul 28, 2017

@bagder, regarding Curl_ossl_seed() changes (silenced warnings with struct curltime ) it is perfectly fine to my view.

@bagder

This comment has been minimized.

Member

bagder commented Jul 30, 2017

Fixed!

@dscho

This comment has been minimized.

Contributor

dscho commented Aug 3, 2017

Thank you! I can confirm that my SSL backends branch, rebased to the latest master, builds in maintainer mode without any complaints with OpenSSL, GNU TLS and Secure Channel support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment