bio_dgram uses recvmsg/sendmsg to retrieve destination and set origin address #5257

mcr · 2018-02-04T19:08:28Z

This code and two test cases adjusts the bss_dgram read/write which is used in DTLS such that
the originating and destination addresses are retrieved upon read, and are set in write. This permits
a UDP "server" socket to be bound to :: (0.0.0.0) and receive traffic to any interface on the machine.

This code includes IP_PKTINFO (v4) for Linux, and IP_RECVDSTADDR/IP_SENDSRCADDR (v4) for *BSD, and if not falls back to recvfrom/sendto(). For IPv6, it is assumed that the stock IETF APIs
using pktinfo are available. The choice of PKTINFO or RECVDSTADDR is determined by the Configure scripts.
These patches include four additional BIO_crtl messages to set/get the origin/dest address from the BIO.
There was previous concern about CMSG_SPACE() being constant or not; but the specifications say that those macros are guaranteed to be compile time constants.

In addition, code to enable IP_PKTINFO and IPV6_PKTINFO on OSX is included. OSX has RECDSTADDR (like other BSDs), but does not have SENDSRCADDR. IP_PKTINFO was added in OSX 10.9, prior versions may need to disable both IP_PKTINFO and IP_RECVDSTADDR.

mcr · 2018-02-04T19:09:49Z

The *BSD patches were tested on a FreeBSD 8.4 machine at ISC.ORG.

FdaSilvaYY · 2018-02-04T23:17:21Z

A lot of code style issue ...
Consider running 'utils\openssl-format-source' script to uncover its.

mcr · 2018-02-05T05:49:09Z

@FdaSilvaYY , code style issues aside, the regression tests seem to all fail without any details. Do you have any idea if there is something systematic wrong? "make tests" works for me.

mcr · 2018-02-05T06:12:07Z

@FdaSilvaYY each patch updated with a run through openssl-format-source. Neither bss_dgram.c nor bio.h were clean before I started, so I included a commit that just fixes them before I edit them.

mattcaswell · 2018-02-05T11:46:36Z

Travis is failing with 3 different errors (in different builds):

../../_srcdist/test/recipes/81-test_bio_write.t ................ 
1..1
    # Subtest: ../../test/bio_write_test
    1..2
        # Subtest: test_bio_write_v4
        1..1
        ok 1 - iteration 1
    ok 1 - test_bio_write_v4
        # Subtest: test_bio_write_v6
        1..1
bind in6fd: Cannot assign requested address
../../util/shlib_wrap.sh ../../test/bio_write_test => 4
not ok 1 - running bio_write_test

ccache gcc  -I. -Icrypto/include -Iinclude -Wall -O3 -pthread -m64 -DDEBUG_UNUSED -DPEDANTIC -pedantic -Wno-long-long -Wall -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wswitch -Wsign-compare -Wmissing-prototypes -Wshadow -Wformat -Wtype-limits -Wundef -Werror -fPIC -DDSO_DLFCN -DHAVE_DLFCN_H -DHAVE_IP_PKTINFO -DNDEBUG -DOPENSSL_NO_STATIC_ENGINE -DOPENSSL_PIC -DOPENSSL_USE_NODELETE -DL_ENDIAN -DOPENSSLDIR="\"/usr/local/ssl\"" -DENGINESDIR="\"/usr/local/lib/engines-1.1\""  -c -MMD -MF crypto/bio/bss_dgram.d.tmp -MT crypto/bio/bss_dgram.o -c -o crypto/bio/bss_dgram.o crypto/bio/bss_dgram.c
crypto/bio/bss_dgram.c: In function 'dgram_read_unconnected_v6':
crypto/bio/bss_dgram.c:462:5: error: C++ style comments are not allowed in ISO C90 [-Werror]
     //unsigned int     chdrlen = CMSG_SPACE(sizeof(struct in6_pktinfo));
     ^
crypto/bio/bss_dgram.c:462:5: error: (this will be reported only once per input file) [-Werror]
cc1: all warnings being treated as errors
make[1]: *** [crypto/bio/bss_dgram.o] Error 1
make[1]: Leaving directory `/home/travis/build/openssl/openssl'
make: *** [all] Error 2
+/// MAKE FAILED

i686-w64-mingw32-gcc  -I. -Icrypto/include -Iinclude -m32 -Wall -O3 -fomit-frame-pointer -Wno-pedantic-ms-format  -DDSO_WIN32 -DOPENSSL_USE_APPLINK -DNDEBUG -DOPENSSL_NO_STATIC_ENGINE -DOPENSSL_PIC -DOPENSSL_BN_ASM_PART_WORDS -DOPENSSL_IA32_SSE2 -DOPENSSL_BN_ASM_MONT -DOPENSSL_BN_ASM_GF2m -DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DRC4_ASM -DMD5_ASM -DRMD160_ASM -DAES_ASM -DVPAES_ASM -DWHIRLPOOL_ASM -DGHASH_ASM -DECP_NISTZ256_ASM -DPADLOCK_ASM -DPOLY1305_ASM -DL_ENDIAN -DWIN32_LEAN_AND_MEAN -DUNICODE -D_UNICODE -D_MT -DOPENSSLDIR="\"/usr/local/ssl\"" -DENGINESDIR="\"/usr/local/lib/engines-1_1\""  -c -MMD -MF crypto/bio/bss_dgram.d.tmp -MT crypto/bio/bss_dgram.o -c -o crypto/bio/bss_dgram.o crypto/bio/bss_dgram.c
crypto/bio/bss_dgram.c:17:23: fatal error: arpa/inet.h: No such file or directory
 #include <arpa/inet.h>
                       ^
compilation terminated.
make[1]: *** [crypto/bio/bss_dgram.o] Error 1
make[1]: Leaving directory `/home/travis/build/openssl/openssl'
make: *** [all] Error 2
+/// MAKE FAILED

The Appveyor builds are failing with a version of the last error.

mattcaswell · 2018-02-05T11:53:43Z

@FdaSilvaYY each patch updated with a run through openssl-format-source. Neither bss_dgram.c nor bio.h were clean before I started, so I included a commit that just fixes them before I edit them.

Don't do that! There is too much "noise" in this patch from the openssl-format-source output. Please back that commit out. If openssl-format-source is not clean to start with then we'll have to handle the style issues manually. Reading the style guide is a good start:

https://www.openssl.org/policies/codingstyle.html

mattcaswell

Skimmed over some parts (there is a lot of noise due to the openssl-format-source). Also didn't look too closely at the tests yet. But there's probably enough comments here for now already!

mattcaswell · 2018-02-05T11:54:20Z

crypto/bio/bss_dgram.c

+#include <arpa/inet.h>
+#include <netinet/in.h>
+#include <netinet/ip6.h>
+#include <netinet/icmp6.h>


This will fail on non-Unix platforms. Probably this needs to be guarded by appropriate ifdef

Is there a way (build-server) on which I can test compile against such a platform?

The only generally available options we have are travis/appveyor integration via github.

crypto/bio/bss_dgram.c

mattcaswell · 2018-02-05T12:00:35Z

crypto/bio/bss_dgram.c

@@ -294,26 +308,262 @@ static void dgram_reset_rcv_timeout(BIO *b)
 # endif
 }

+# if defined(HAVE_IP_PKTINFO)


Rather than creating a custom define here, could we not just use __linux__? Adding new defines to Configure just for this seems quite heavyweight.

mattcaswell · 2018-02-05T12:04:11Z

crypto/bio/bss_dgram.c

+    mhdr.msg_iov = &iov;
+    mhdr.msg_iovlen = 1;
+
+    if (dstaddr) {


Style: if (dstaddr != NULL)

mattcaswell · 2018-02-05T12:05:00Z

crypto/bio/bss_dgram.c

+        mhdr.msg_controllen = sizeof(chdr);
+    }
+
+    pkt_info = NULL;


Just do this in the declaration

mattcaswell · 2018-02-05T12:24:41Z

crypto/bio/bss_dgram.c

+        struct sockaddr *sa = (struct sockaddr *)BIO_ADDR_sockaddr(&data->peer);
+        if (data->addr.sa.sa_family == 0) {
+            dgram_get_sockname(b);
+        }


Don't use {} for just a single line if.

mattcaswell · 2018-02-05T12:25:37Z

crypto/bio/bss_dgram.c

+        ret = BIO_ADDR_sockaddr_size(&data->addr);
+        if (ret == 0) {         /* never set, retrieve it */
+            ret = dgram_get_sockname(b);
+        }


Remove {}

mattcaswell · 2018-02-05T12:26:01Z

crypto/bio/bss_dgram.c

+        }
+
+        /* FIXME: if num < ret, we will only return part of an address.
+           That should bee an error, no? */


Needs resolution

Not my code actually. Happy to fix this in separate patch.

mattcaswell · 2018-02-05T12:27:16Z

test/bio_read_test.c

+#include <openssl/bio.h>
+#include <netinet/in.h>
+#include <sys/socket.h>
+#include <unistd.h>


Won't work on windows!

If I have ifdef the entire test. Not sure what is the best way to test these things on windows.

mattcaswell · 2018-02-05T12:29:47Z

test/bio_read_test.c

+    BIO *out;
+    int expected_count = PACKET_COUNT;
+
+    pid_t pid = fork();


fork() doesn't exist on Windows!

@mattcaswell, my run of the tests on travis seemed to just die in the middle without any obvious error. I guess AppVeyor builds for Windows?
I also now recall that travis has no IPv6. Not sure what to do about that yet.
I don't want to rewrite this test case with select(), because that will just be more complicated and won't work on Windows anyway. Also, re, //- comments, shouldn't the default flags warn on that if travis builds break on that?
Finally, 90% of the style issues you complain about were introduced by openssl-format-source. So I think that openssl-format-source does not comply to the style guide.
What I did was to run it without any material changes and then commit those as on their own so that my changes would be not obscured by indent changes. Why is it wrong to fix bss_dgram.c and bio.h's indented?

I guess AppVeyor builds for Windows?

Yes

I also now recall that travis has no IPv6. Not sure what to do about that yet.

Possibly if we can't create the sockets then we skip that bit of the test - but don't fail it.

Also, re, //- comments, shouldn't the default flags warn on that if travis builds break on that?

The default flags are designed for the general user - not people developing patches. OpenSSL has to run on lots of different platforms with lots of different compilers. We don't run the default with too high warning options because that will introduce spurious build failures for the general user. If you are developing patches you should use --strict-warnings as mentioned in the CONTRIBUTING file.

Finally, 90% of the style issues you complain about were introduced by openssl-format-source. So I think that openssl-format-source does not comply to the style guide.

Probably it doesn't. The style has developed further since that was written. I wouldn't recommend using it myself. As I said above I would back out those changes.

Why is it wrong to fix bss_dgram.c and bio.h's indented?

It generates a lot of noise and makes it difficult to review the patch (difficult to distinguish your changes from auto-generated changes). If we want to fix large amounts of the formatting in files it should be done in a dedicated PR. Localised fixes are ok.

I have removed the patch that just ran openssl-format-source on that code.

mcr · 2018-03-08T19:55:04Z

https://travis-ci.org/openssl/openssl/jobs/350967905
@levitte this worked earlier this morning! Maybe before I rebased...

    
Operating system: x86_64-whatever-linux2
/usr/bin/env __CNF_CPPDEFINES='' __CNF_CPPINCLUDES='' __CNF_CPPFLAGS='' __CNF_CFLAGS='' __CNF_CXXFLAGS='' __CNF_LDFLAGS='' __CNF_LDLIBS='' /usr/bin/perl ./Configure linux-x86_64 'no-asm' '-Werror' '--debug' 'no-afalgeng' 'no-shared' 'enable-crypto-mdebug' 'enable-rc5' 'enable-md2'
***** Mixing env / make variables and additional compiler/linker flags as
***** configure command line option is not permitted.
***** Affected env / make variables: CFLAGS, CXXFLAGS


0.00s$ ./configdata.pm --dump
/home/travis/.travis/job_stages: line 57: ./configdata.pm: No such file or directory
The command "./configdata.pm --dump" failed and exited with 127 during .

mattcaswell · 2018-03-09T10:00:53Z

this worked earlier this morning! Maybe before I rebased...

Rebase again to pick up fb174fa?

mcr · 2018-03-09T12:43:27Z

all green. review comments dealt with.

mattcaswell

As a general comment there are lots of indentation errors in this PR. I didn't call them out individually. Please always use 4 spaces for indentation. There are numerous cases of a different number of spaces being used, or tab characters being used.

mattcaswell · 2018-03-09T12:50:52Z

crypto/bio/bss_dgram.c

+#else
+#include <sys/socket.h>
+#include <netinet/in.h>
+#include <netinet/ip6.h>


We still need to be able handle non-windows and non-unix/linux targets.

This remains an issue. It's unclear to me whether all of these headers will be available on all platforms. At least in some files we wrap some of these in an "ifdef OPENSSL_SYS_UNIX". We have, as yet, never used netinet/ip6.h - so I am concerned it may not be universally available on all the platforms that we might need to support.

mattcaswell · 2018-03-09T12:53:42Z

crypto/bio/bss_dgram.c

+                                     BIO_ADDR *dstaddr, BIO_ADDR *peer)
+{
+    int len = 0;
+    unsigned char    chdr[BIO_CMSG_ADDR_SIZE];


Too many spaces

mattcaswell · 2018-03-09T13:01:36Z

crypto/bio/bss_dgram.c

+    if(dstaddr != NULL) {
+      memset(chdr, 0, sizeof(chdr));
+      cmsg = (struct cmsghdr *) chdr;
+      mhdr.msg_control = (void *) cmsg;


Style nit: no space between variable and cast in above two lines

mattcaswell · 2018-03-09T13:03:43Z

crypto/bio/bss_dgram.c

+    val = 1;
+    if(setsockopt(b->num, IPPROTO_IP, IP_PKTINFO, &val, sizeof(val)) < 0) {
+      return -1;
+    }


Style nit: No {} around a single line if

mattcaswell · 2018-03-09T13:04:31Z

crypto/bio/bss_dgram.c

+	{
+            if (cmsg->cmsg_level != IPPROTO_IP)
+          	continue;
+            switch(cmsg->cmsg_type) {


switch seems an odd choice when there is only one case

mattcaswell · 2018-03-09T13:33:07Z

crypto/bio/bss_dgram.c

+    mhdr.msg_iovlen = 1;
+
+    srcaddr = (struct sockaddr_in *)BIO_ADDR_sockaddr(&data->addr);
+    if(srcaddr && srcaddr->sin_addr.s_addr != 0) {


Use srcaddr != NULL

mattcaswell · 2018-03-09T13:34:20Z

crypto/bio/bss_dgram.c

+    struct cmsghdr *cmsg;
+    struct iovec iov;
+    char chdr[BIO_CMSG_PKT6_SIZE];
+    bio_dgram_data *data = (bio_dgram_data *)b->ptr;


Blank line between declarations and code

Comment is still relevant

mattcaswell · 2018-03-09T13:37:08Z

test/bio_read_test.c

+  }
+#if 0
+  printf("bound to port: %u\n", ntohs(localhost.sin_port));
+#endif


Remove this

mattcaswell · 2018-03-09T13:38:41Z

test/bio_read_test.c

+
+  if(ret <= 0) {
+    test_printf_stderr("BIO_read returned %d\n", ret);
+  }


No {} around single line if

This comment still relevant

Also please use TEST_info for informational messages.

mattcaswell · 2018-03-09T13:44:21Z

test/bio_write_test.c

+#include <openssl/bio.h>
+#include <openssl/rand.h>
+#include <openssl/bio.h>
+


This file should be using the test framework more fully, i.e. using the TEST_ macros as defined in testutil.h. Take a look at the other tests for examples.

This comment is still relevant.

mcr · 2018-03-15T09:37:07Z

Thank you for the patience with my style challenges. Every project has different demands... mattcaswell <notifications@github.com> wrote: We still need to be able handle non-windows and non-unix/linux targets. which ones specifically, and how can I test on them? + switch(cmsg->cmsg_type) { switch seems an odd choice when there is only one case yes, perhaps, until one adds another second item. I feel that this is stylistically what people expect to see when processing a the control messages. But, I will change it. ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ In crypto/bio/bss_dgram.c: > + iov.iov_base = (caddr_t) in; + + memset(&mhdr, 0, sizeof(mhdr)); + mhdr.msg_name = (caddr_t)BIO_ADDR_sockaddr(peer); + mhdr.msg_namelen = sizeof(struct sockaddr_in); + mhdr.msg_iov = &iov; + mhdr.msg_iovlen = 1; + + if(dstaddr != NULL) { + memset(chdr, 0, sizeof(chdr)); + cmsg = (struct cmsghdr *) chdr; + mhdr.msg_control = (void *) cmsg; + mhdr.msg_controllen = sizeof(chdr); + } + + if((len = recvmsg(b->num, &mhdr, 0)) >= 0) { Perhaps add && dstaddr != NULL in this expression? No, because if the caller didn't set dstaddr, then they aren't interested in it having it returned. The message should still be retrieved. In crypto/bio/bss_dgram.c: > + iov.iov_base = (caddr_t) in; + + memset(&mhdr, 0, sizeof(mhdr)); + mhdr.msg_name = (caddr_t)BIO_ADDR_sockaddr(peer); + mhdr.msg_namelen = sizeof(struct sockaddr_in); + mhdr.msg_iov = &iov; + mhdr.msg_iovlen = 1; + + if(dstaddr != NULL) { + memset(chdr, 0, sizeof(chdr)); + cmsg = (struct cmsghdr *) chdr; + mhdr.msg_control = (void *) cmsg; + mhdr.msg_controllen = sizeof(chdr); + } + + dstaddr = NULL; Why are you doing this? It's wrong, I have removed it. Thank you for noticing. In crypto/bio/bss_dgram.c: > + /* see if we found something */ + if(pkt_info != NULL && dstaddr != NULL) { + dstaddr->s_in.sin_family = AF_INET; + dstaddr->s_in.sin_addr = pkt_info->ipi_addr; + } + } + + /* NOTE: peer was filled in by kernel */ + return len; +} +#elif defined(HAVE_IP_RECVDSTADDR) +/* FREEBSD, DragonFly, NetBSD, OpenBSD, probably BSDi */ +/* implies IP_SENDSRCADDR is available too */ +static int dgram_read_unconnected_v4(BIO *b, const char *in, int inl, + int flags, + BIO_ADDR *dstaddr, BIO_ADDR *peer) This function seems to be almost identical to the once above with only minor differences. Can't we combine the two and just have guards around the bits which are different? Once the differences in declarations are accounted for with #ifdefs or macros somewhere, it seems to me that the flow will be impacted such that observations like the one that you made above would not be seen. ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ In crypto/bio/bss_dgram.c: > + mhdr.msg_iov = &iov; + mhdr.msg_iovlen = 1; + + if(dstaddr != NULL) { + memset(chdr, 0, sizeof(chdr)); + cmsg = (struct cmsghdr *) chdr; + mhdr.msg_control = (void *) cmsg; + mhdr.msg_controllen = sizeof(chdr); + } + + dstaddr = NULL; + if((len = recvmsg(b->num, &mhdr, 0)) >= 0) { + for (cmsg = CMSG_FIRSTHDR(&mhdr); + cmsg != NULL; + cmsg = CMSG_NXTHDR(&mhdr, cmsg)) + { { goes on the end of the previous line ok. +#endif + +/* on Windows, RFC3542 is not implemented correctly, instead + * WSARecvMsg must be used rather than recvmsg But you don't seem to use that below? No, I have not the skill or equipment to implement that. +static int dgram_get_sockname(BIO *b) +{ + bio_dgram_data *data = NULL; + socklen_t addr_len = sizeof(bio_dgram_data); + + data = (bio_dgram_data *)b->ptr; Do this initialisation in the declaration of data above. done. + srcaddr = (struct sockaddr_in *)BIO_ADDR_sockaddr(&data->addr); + if(srcaddr && srcaddr->sin_addr.s_addr != 0) { + struct in_pktinfo *pkt_info; + memset(chdr, 0, sizeof(chdr)); Add blank line between declarations and start of code done. + cmsg = CMSG_FIRSTHDR(&mhdr); + cmsg->cmsg_len = CMSG_LEN(sizeof(struct in_pktinfo)); Too many spaces I was aligning the assignments up, but if that's not something you like, then I'll fix them all. +#if 0 + printf("controllen: %d\n", mhdr.msg_controllen); +#endif Please remove this done. + /* CMSG_SOCKET is from sys/socket.h, and makes + * space for a cmsghdr as well as alignment + */ + char chdr[BIO_CMSG_ADDR_SIZE]; + bio_dgram_data *data = (bio_dgram_data *)b->ptr; Blank line between declarations and code done. + struct sockaddr_in *srcaddr; + struct in_addr *origaddr; Too many spaces ok. + if(srcaddr && srcaddr->sin_addr.s_addr != 0) { Use srcaddr != NULL done. +#endif + +#if defined(AF_INET6) && !defined(_WIN32) +static int dgram_write_unconnected_v6(BIO *b, const char *out, int outl) +{ + struct sockaddr_in6 addr; + struct sockaddr_in6 *srcaddr; + struct msghdr mhdr; + struct cmsghdr *cmsg; + struct iovec iov; + char chdr[BIO_CMSG_PKT6_SIZE]; + bio_dgram_data *data = (bio_dgram_data *)b->ptr; Blank line between declarations and code it's all declarations. In test/bio_read_test.c: +#if 0 + printf("bound to port: %u\n", ntohs(localhost.sin_port)); +#endif Remove this done. ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ In test/bio_read_test.c: > + + ba = BIO_ADDR_new(); + + in = BIO_new_dgram(fd, BIO_CLOSE); + + while(--count > 0 && (ret=BIO_read(in, buf, 512))>0) { + + /* now check out the bio structure for the origin of the packet */ + BIO_get_dgram_origin(in, ba); + port = ntohs(BIO_ADDR_rawport(ba)); + if(port!=0 && port != portnum) { exit(5); } + } + + if(ret <= 0) { + test_printf_stderr("BIO_read returned %d\n", ret); + } No {} around single line if ok. ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ In test/bio_write_test.c: > @@ -0,0 +1,228 @@ +/* + * Copyright 2018 The OpenSSL Project Authors. All Rights Reserved. + * + * Licensed under the OpenSSL license (the "License"). You may not use + * this file except in compliance with the License. You can obtain a copy + * in the file LICENSE in the source distribution or at + * https://www.openssl.org/source/license.html + */ +#include <stdio.h> +#include <string.h> +#include <openssl/evp.h> +#include <openssl/bio.h> +#include <openssl/rand.h> +#include <openssl/bio.h> + This file should be using the test framework more fully, i.e. using the TEST_ macros as defined in testutil.h. Take a look at the other tests for examples. Are saying something like: TEST_int_ne(count, 0); rather than: if(count != 0) { test_printf_stderr("failed receive all packets: %d\n", count); exit(2); } if so I will see what I can in another commit. This email will likely arrive before I have a chance to push any code.

…nation address seen

Includes a test case to write packets using the interface.

…_PKTINFO, and set this option generically on Linux

…figuration

…his author. Make sure it compiles on Windows using recvfrom()/sendto() instead

…tainers have IPv6 disabled (not even a loopback) so no IPv6 tests will function, turn off IPv6 version of bio_write_test and bio_read_test

…SENDSRCADDR

not have SENDDSTADDR for IPv4. RFC 3542 needs to be explicitely enabled with a #define clang does not believe that Darwin CMSG_SPACE is a constant and issues warning

mspncp · 2022-03-31T21:39:53Z

crypto/bio/bss_dgram.c

+# if OPENSSL_USE_IPV6
+#  if OPENSSL_SYS_WINDOWS
+#   include "shared/netiodef.h>
+#  else
+#   include <netinet/ip6.h>
+#  endif
+# endif


Referring to your discussion with Michael Wojcik:

> > #if defined OPENSSL_SYS_WINDOWS > > # include <shared/netiodef.h> > > #else > > # include <netinet/ip6.h> > > #endif > > But, don't all the OPENSSL_* macros expand to 0/1, anyway, so we actually > just want #if OPENSSL_SYS_WINDOWS?

In principle, you might be right, but in practice a quick git grep OPENSSL_SYS will convince you that everywhere throughout the OpenSSL source code we use either #ifdef OPENSSL_SYS* or #if defined(OPENSSL_SYS*) and not #if OPENSSL_SYS*. So for consistency reasons I ask you to please change it.

(The OPENSSL_USE_IPV6 macro unfortunately has a different semantics (see a5a0f32), here you should keep the #if. So much for consistency :-/ )

… when dgram is disabled

mcr force-pushed the bio_dgram_uses_recvmsg branch from 2695456 to c0fdc34 Compare February 4, 2018 19:12

mcr force-pushed the bio_dgram_uses_recvmsg branch from c0fdc34 to 2df6c9c Compare February 5, 2018 06:11

mattcaswell requested changes Feb 5, 2018

View reviewed changes

mcr force-pushed the bio_dgram_uses_recvmsg branch from 2df6c9c to baf995a Compare February 6, 2018 01:47

mcr force-pushed the bio_dgram_uses_recvmsg branch from baf995a to 5756804 Compare February 25, 2018 19:42

mcr force-pushed the bio_dgram_uses_recvmsg branch 7 times, most recently from 49cce69 to 2166e8c Compare March 8, 2018 18:54

mcr force-pushed the bio_dgram_uses_recvmsg branch from 2166e8c to f48d0d9 Compare March 8, 2018 22:19

mattcaswell requested changes Mar 9, 2018

View reviewed changes

mattcaswell added this to the Post 1.1.1 milestone Mar 20, 2018

richsalz added after 1.1.1 and removed after 1.1.1 labels May 6, 2018

mspncp removed the after 1.1.1 label Oct 25, 2019

t8m modified the milestones: Post 1.1.1, Post 3.0.0 May 12, 2021

mcr added 21 commits March 29, 2022 13:44

added BIO_CTRL_DGRAM_SET_ADDR.BIO_CTRL_DGRAM_GET_ADDR to record desti…

957f2e9

…nation address seen

bio_write() using sendmsg(2) for IPv4 and IPv6, on Linux.

651c947

Includes a test case to write packets using the interface.

bio_write test now sends to different destinations

63c0b8d

have bio_write/bio_read make use of IP_PKTINFO dependant upon HAVE_IP…

ed607c0

…_PKTINFO, and set this option generically on Linux

bio_write uses *BSD IP_SENDSRCADDR code with #ifdef controlled by con…

fb16fdc

…figuration

bio_read()/bio_write() needs to use WSARecvMsg, but not testable to t…

2a8259f

…his author. Make sure it compiles on Windows using recvfrom()/sendto() instead

bio_read/write tests need to check TRAVISCI_NO_IPV6, as travis-ci con…

3f33aae

…tainers have IPv6 disabled (not even a loopback) so no IPv6 tests will function, turn off IPv6 version of bio_write_test and bio_read_test

do FreeBSD version of dgram_read and dgram_write using RECVDSTADDR / …

ca8658d

…SENDSRCADDR

use HAVE_IP_PKTINFO on Darwin builds (>10.9) as Darwin does

c087900

not have SENDDSTADDR for IPv4. RFC 3542 needs to be explicitely enabled with a #define clang does not believe that Darwin CMSG_SPACE is a constant and issues warning

use bio_local.h rather than lcl.h, set __USE_GNU conditionally.

c95f270

do not compile or run bio_read_test or bio_write_test if no-dgram set

a3c2738

added man page for BIO_dgram_{set,get}_{origin,dest}

11e5962

adjust code for nits, and coding style

b4b3880

rework and refactor examples so that they compile

c64934e

fold in the check for dstaddr/dstrecv into recvmsg test

be27538

refactor read_unconnected_v4 to have common loop

f74205a

TEST_ macros do not exit function, need to exit function after they fail

a55c832

use signed integers big enough for port numbers and -1

a841392

mark all sockets as REUSEPORT

35e53cf

fix build.info: if sock not defined, then dgram will not be defined

93d1de0

added man page for BIO_dgram_{set,get}_{origin,dest}

3567dbb

mcr force-pushed the bio_dgram_uses_recvmsg branch from 371fea6 to 3567dbb Compare March 29, 2022 17:44

mspncp reviewed Mar 31, 2022

View reviewed changes

mcr added 2 commits April 6, 2022 18:09

consistently use OPENSSL_USE_IPV6 rather than AF_INET6

54dedd1

if v6 bind fails, then make sure to free dsthost1/dsthost2

241af47

mcr force-pushed the bio_dgram_uses_recvmsg branch from a8ac8b5 to 7a1934a Compare April 6, 2022 22:10

include OpenSSL::Test, to get plan skip_all, perhaps to skip properly…

d8c3d89

… when dgram is disabled

mcr force-pushed the bio_dgram_uses_recvmsg branch from 7a1934a to d8c3d89 Compare April 7, 2022 02:42

fixup! added man page for BIO_dgram_{set,get}_{origin,dest}

f20d7f0

hlandau mentioned this pull request Apr 28, 2022

BIO_dgram API evolution #18200

Closed

bio_dgram uses recvmsg/sendmsg to retrieve destination and set origin address #5257

Are you sure you want to change the base?

bio_dgram uses recvmsg/sendmsg to retrieve destination and set origin address #5257

Conversation

mcr commented Feb 4, 2018 • edited

mcr commented Feb 4, 2018 • edited

FdaSilvaYY commented Feb 4, 2018

mcr commented Feb 5, 2018

mcr commented Feb 5, 2018

mattcaswell commented Feb 5, 2018

mattcaswell commented Feb 5, 2018

mattcaswell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcr commented Mar 8, 2018

mattcaswell commented Mar 9, 2018

mcr commented Mar 9, 2018

mattcaswell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcr commented Mar 15, 2018 via email

Choose a reason for hiding this comment

mcr commented Feb 4, 2018 •

edited

mcr commented Feb 4, 2018 •

edited