Fix Building Using MSVC

config-msvc.h +0 -0

  #ifdef HAVE_CONFIG_MSVC_LOCAL_H
  #include <config-msvc-local.h>

src/openvpn/crypto_openssl.c +2 -0

index 5549d70..bed39f3 100644
@@ -287,7 +287,12 @@ show_available_ciphers()
 
      /* If we ever exceed this, we must be more selective */
      const size_t cipher_list_len = 1000;
+#ifdef _MSC_VER
+    //While GCC will allow you to use a const, MSVC won't (at least 

with a simple declaration). Use a compile time constant for now

src/openvpn/ssl_openssl.h +0 -0

index c64c65f..a974e7e 100644

Quoted 13 lines · Eric Thorpe

  #ifdef HAVE_CONFIG_MSVC_LOCAL_H
  #include <config-msvc-local.h>

src/openvpn/crypto_openssl.c +2 -0

index 5549d70..bed39f3 100644
@@ -287,7 +287,12 @@ show_available_ciphers()
 
      /* If we ever exceed this, we must be more selective */
      const size_t cipher_list_len = 1000;
+#ifdef _MSC_VER
+    //While GCC will allow you to use a const, MSVC won't (at least 

with a simple declaration). Use a compile time constant for now

src/openvpn/crypto_openssl.c show_available_ciphers() · line 296

Quoted 5 lines

src/openvpn/crypto_openssl.c +3 -0

+    const EVP_CIPHER *cipher_list[1000];
+#else
      const EVP_CIPHER *cipher_list[cipher_list_len];
+#endif
      size_t num_ciphers = 0;

I don't think we should do this. If we have a length, use it. If the compiler is too daft to understand consts, maybe we should go for "#define CIPHER_LIST_LEN 1000" instead - indeed, more changes, but no magic numbers. Or abandon the definition, use [1000], and sizeof() in the out of bounds check below.

Steffan, your code, what would you say?

Hi,

thanks for your patch. I'm not exactly happy with it for a number of reasons, but it's a good way to get started.

Details...

On Tue, Mar 14, 2017 at 09:26:52AM +1100, Eric Thorpe wrote:
[..]

config-msvc-version.h.in

Quoted 4 lines

config-msvc-version.h.in +0 -0

index 4bc89e7..7977cb8 100644

I can't comment on these changes, but I assume that's "funny macro handling" related... and since it's MSVC only, the risk of breaking anything else is small.

config-msvc.h typedef __int64 int64_t; · line 129

Quoted 9 lines

config-msvc.h +1 -0

index 3e71c85..9b97e71 100644
@@ -126,6 +126,7 @@ typedef __int64 int64_t;
   typedef __int32 int32_t;
   typedef __int16 int16_t;
   typedef __int8 int8_t;
+typedef uint16_t in_port_t;

The indenting of that chunk looks a bit weird (extra space at the beginning of the context lines) but that's easily sorted manually.

Quoted 13 lines

config-msvc.h +0 -0

   #ifdef HAVE_CONFIG_MSVC_LOCAL_H
   #include <config-msvc-local.h>

src/openvpn/crypto_openssl.c +2 -0

index 5549d70..bed39f3 100644
@@ -287,7 +287,12 @@ show_available_ciphers()
 
       /* If we ever exceed this, we must be more selective */
       const size_t cipher_list_len = 1000;
+#ifdef _MSC_VER
+    //While GCC will allow you to use a const, MSVC won't (at least

with a simple declaration). Use a compile time constant for now

src/openvpn/crypto_openssl.c

Quoted 5 lines

src/openvpn/crypto_openssl.c +3 -0

+    const EVP_CIPHER *cipher_list[1000];
+#else
       const EVP_CIPHER *cipher_list[cipher_list_len];
+#endif
       size_t num_ciphers = 0;

I don't think we should do this. If we have a length, use it. If the compiler is too daft to understand consts, maybe we should go for "#define CIPHER_LIST_LEN 1000" instead - indeed, more changes, but no magic numbers. Or abandon the definition, use [1000], and sizeof() in the out of bounds check below.

Steffan, your code, what would you say?

Style-wise: please do not use C++ comments, and break long lines.

src/openvpn/openvpn.vcxproj line 104

Quoted 9 lines

src/openvpn/openvpn.vcxproj +2 -0

@@ -99,13 +99,16 @@
       </Link>
     </ItemDefinitionGroup>
     <ItemGroup>
+    <ClCompile Include="argv.c" />
       <ClCompile Include="base64.c" />
+    <ClCompile Include="block_dns.c" />

Yeah. Oversight when we added new modules - these are easy enough :-)

Quoted 5 lines

src/openvpn/ssl_openssl.h +0 -0

index c64c65f..a974e7e 100644

src/openvpn/ssl_openssl.h line 58

Quoted 10 lines

src/openvpn/ssl_openssl.h +4 -0

@@ -49,7 +49,11 @@
    */
   struct tls_root_ctx {
       SSL_CTX *ctx;
+#ifdef _MSC_VER
+    struct timeval crl_last_mtime;
+#else
       struct timespec crl_last_mtime;
+#endif
       off_t crl_last_size;
   };

We should not do this. It's not your fault for finding this, but having looked at the code to understand why this would be fixing things, I wonder why we're using a "struct timespec" in the first place, or why we're having a structure "with subseconds" in there when all we use is the "seconds" bit *because* "windows has no nanoseconds"...

src/openvpn/ssl.c

     /*
      * Store the CRL if this is the first time or if the file was changed since
      * the last load.
      * Note: Windows does not support tv_nsec.
      */
     if ((ssl_ctx->crl_last_size == crl_stat.st_size)
         && (ssl_ctx->crl_last_mtime.tv_sec == crl_stat.st_mtime))
...
     ssl_ctx->crl_last_mtime.tv_sec = crl_stat.st_mtime;
     ssl_ctx->crl_last_size = crl_stat.st_size;

Antonio, this is your code - how shall we handle this?

Today, we only use seconds, so "struct timeval" on all platforms would be good enough. Some platforms' "struct stat" actually has a st_mtim which is a "struct timespec" (and st_mtime is defined as st_mtim.tv_sec), so if we ever go for subsecond accuracy, timespec is the right thing to do - OTOH, that will need at least one extra configure test, given how various OSes <sys/stat.h> weasle around "struct timespec"...

#if (_POSIX_C_SOURCE - 0) >= 200809L || (_XOPEN_SOURCE - 0) >= 700 || \
defined(_NETBSD_SOURCE)
struct timespec st_atim; /* time of last access */
struct timespec st_mtim; /* time of last data modification */
struct timespec st_ctim; /* time of last file status change */
struct timespec st_birthtim; /* time of creation */
#else

         time_t    st_atime;             /* time of last access */
         long      st_atimensec;         /* nsec of last access */
         time_t    st_mtime;             /* time of last data modification */
         long      st_mtimensec;         /* nsec of last data modification */

...

So, it might be worth considering just dropping the notion of interest in subseconds, and go for a plain "time_t" for "crl_last_mtime"... (slightly more code changes, but less portability hassle).

Again, Antonio, your call...

Eric, could you re-send the patch without the crypto_openssl.c and ssl_openssl.h hunks? I could merge the rest while Steffan and Antonio ponder these changes...

gert

Quoted 5 lines

src/openvpn/ssl_openssl.h +0 -0

index c64c65f..a974e7e 100644

src/openvpn/ssl_openssl.h line 58

Quoted 10 lines

src/openvpn/ssl_openssl.h +4 -0

@@ -49,7 +49,11 @@
   */
  struct tls_root_ctx {
      SSL_CTX *ctx;
+#ifdef _MSC_VER
+    struct timeval crl_last_mtime;
+#else
      struct timespec crl_last_mtime;
+#endif
      off_t crl_last_size;
  };

We should not do this. It's not your fault for finding this, but having looked at the code to understand why this would be fixing things, I wonder why we're using a "struct timespec" in the first place, or why we're having a structure "with subseconds" in there when all we use is the "seconds" bit *because* "windows has no nanoseconds"...

src/openvpn/ssl.c

    /*
     * Store the CRL if this is the first time or if the file was changed since
     * the last load.
     * Note: Windows does not support tv_nsec.
     */
    if ((ssl_ctx->crl_last_size == crl_stat.st_size)
        && (ssl_ctx->crl_last_mtime.tv_sec == crl_stat.st_mtime))
...
    ssl_ctx->crl_last_mtime.tv_sec = crl_stat.st_mtime;
    ssl_ctx->crl_last_size = crl_stat.st_size;

Antonio, this is your code - how shall we handle this?

Today, we only use seconds, so "struct timeval" on all platforms would be good enough. Some platforms' "struct stat" actually has a st_mtim which is a "struct timespec" (and st_mtime is defined as st_mtim.tv_sec), so if we ever go for subsecond accuracy, timespec is the right thing to do - OTOH, that will need at least one extra configure test, given how various OSes <sys/stat.h> weasle around "struct timespec"...

#if (_POSIX_C_SOURCE - 0) >= 200809L || (_XOPEN_SOURCE - 0) >= 700 || \
defined(_NETBSD_SOURCE)
struct timespec st_atim; /* time of last access */
struct timespec st_mtim; /* time of last data modification */
struct timespec st_ctim; /* time of last file status change */
struct timespec st_birthtim; /* time of creation */
#else

        time_t    st_atime;             /* time of last access */
        long      st_atimensec;         /* nsec of last access */
        time_t    st_mtime;             /* time of last data modification */
        long      st_mtimensec;         /* nsec of last data modification */

...

So, it might be worth considering just dropping the notion of interest in subseconds, and go for a plain "time_t" for "crl_last_mtime"... (slightly more code changes, but less portability hassle).

Again, Antonio, your call...

If there is no objection, I'll send a patch for this.