On Mon, 2020-04-20 at 10:22 -0700, Daniel Lenski wrote: > Good catch. It appears this broke in > https://gitlab.com/openconnect/openconnect/-/commit/7cb8996e21b442c4ec60ce25c87e8a69516fac17#05d8493af0b3a0d467325299d974b0949981595d_189_189, > when David cleaned up the protocol-enumerating code and removed the > empty/null protocol definition from the end of the list. The problem > is that the `print_supported_protocols` function *wasn't* modified > here… urk. > > I'm a little bit mystified about why this appears to *continue to > work* on Linux, which is why we haven't noticed it, even though it > causes the expected SIGSEGV on Solaris. My guess is that Linux's > `calloc` allocates more zero bytes than requested, silently hiding the > problem. Shows up if you run in valgrind though: --protocol=pulse Compatible with Pulse Connect Secure SSL VPN ==30263== Invalid read of size 8 ==30263== at 0x10FD76: print_supported_protocols_usage (main.c:680) ==30263== by 0x10FD76: usage (main.c:807) ==30263== by 0x10C05C: main (main.c:1810) ==30263== Address 0xc049f90 is 0 bytes after a block of size 128 alloc'd ==30263== at 0x4C31B25: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==30263== by 0x4E4D00B: openconnect_get_supported_protocols (library.c:198) ==30263== by 0x10FD04: print_supported_protocols_usage (main.c:678) ==30263== by 0x10FD04: usage (main.c:807) ==30263== by 0x10C05C: main (main.c:1810) ==30263== > I've created a MR to properly fix this: > https://gitlab.com/openconnect/openconnect/-/merge_requests/94 Nah, if openconnect_get_supported_protocols() originally returned an array with the sentinel included, then that is its ABI. We broke that. Fixing up our own usage of that ABI doesn't actually excuse changing the ABI. And yes, one of the two callers in NetworkManager-openconnect does depend on the sentinel. We have to put it back. I think we *can* get away with returning the correct value (e.g. 4 now) rather than the 5 we used to return before commit 7cb8996e21. Even though that's strictly an ABI change it shoujld be OK.
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
_______________________________________________ openconnect-devel mailing list openconnect-devel@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/openconnect-devel
