Re: openconnect-8.0[568] on Solaris dumps core in print_supported_protocols_usage (main.c:674)
Updated MR to restore the sentinel value to the array returned by `openconnect_get_supported_protocols`, and also verified that the Java library was *not* affected, insofar as it used the length returned rather than relying on the sentinel value. -Dan On Mon, Apr 20, 2020 at 11:14 AM Daniel Lenski wrote: > > On Mon, Apr 20, 2020 at 11:05 AM David Woodhouse wrote: > > Shows up if you run in valgrind though: > > Yes indeed. Wondering if there's a simple way to add automated > valgrind testing à la Coverity. > > > > 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. > > Right. I was thinking we might get away with it due to the small > number of (known) users, but not worth it. > > > 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. > > Makes sense. Will update the MR. > > > 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. > > Agreed, that was basically an off-by-1 bug by me. Hopefully no one is > has reclassified it as an off-by-1 feature. :-P > > Dan ___ openconnect-devel mailing list openconnect-devel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/openconnect-devel
Re: openconnect-8.0[568] on Solaris dumps core in print_supported_protocols_usage (main.c:674)
On Mon, Apr 20, 2020 at 11:05 AM David Woodhouse wrote: > Shows up if you run in valgrind though: Yes indeed. Wondering if there's a simple way to add automated valgrind testing à la Coverity. > > 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. Right. I was thinking we might get away with it due to the small number of (known) users, but not worth it. > 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. Makes sense. Will update the MR. > 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. Agreed, that was basically an off-by-1 bug by me. Hopefully no one is has reclassified it as an off-by-1 feature. :-P Dan ___ openconnect-devel mailing list openconnect-devel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/openconnect-devel
Re: openconnect-8.0[568] on Solaris dumps core in print_supported_protocols_usage (main.c:674)
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=pulseCompatible 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. smime.p7s Description: S/MIME cryptographic signature ___ openconnect-devel mailing list openconnect-devel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/openconnect-devel
Re: openconnect-8.0[568] on Solaris dumps core in print_supported_protocols_usage (main.c:674)
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.
I've created a MR to properly fix this:
https://gitlab.com/openconnect/openconnect/-/merge_requests/94
-Dan
On Mon, Apr 20, 2020 at 9:01 AM Thomas Hildebrandt
wrote:
>
> All,
>
> in recent versions of openconncet, I observe a SEGV error in
> print_supported_protocols_usage() when running openconnect --help.
>
> Present analysis seems to point to the way the code iterates over the
> array of protocol information ("protos") in this statement:
>
> 680 for (p=protos; p->name; p++)
> 681 printf(" --protocol=%-16s %s%s\n",
> 682p->name, p->description, p==protos
> ? _(" (default)") : "");
>
> The issue is that this loop doesn't terminate as intended due to the way
> the protos array is filled. p->name never becomes 0, so the loop tries
> to iterate beyond protos' boundaries, causing the SEGV.
>
> The following fix helped this (at least for me, and there might be
> better ways)
>
> 674 static void print_supported_protocols_usage(void)
> 675 {
> 676 struct oc_vpn_proto *protos, *p;
> 677 int ret;
> 678
> 679 if ((ret = openconnect_get_supported_protocols())>=0) {
> 680 printf("\n%s:\n", _("Set VPN protocol"));
> 681 for (p=protos; /*p->name,*/ ret>0 ; p++,ret--)
> 682 printf(" --protocol=%-16s %s%s\n",
> 683p->name, p->description, p==protos
> ? _(" (default)") : "");
>
> Unfortunately I cannot tell whether this problem exists on other
> platforms as well - my Linux boxes miss OpenSSL and I presently have
> limited network access only. The suggested fix should work on all
> platforms, though.
>
> Pls. cc: me in replies (if any), as I am not subscribed to the list.
>
> TIA, kind regards,
> - Thomas
> --
> Thomas Hildebrandtmailto:Thomas.Hildebrandt(at)Oracle.COM
> Senior Field Engineer
> Oracle EMEA Systems Support
>
> Oracle Deutschland B.V. & Co KGhttp://www.oracle.com
> Neue Mainzer Strasse 46-50http://www.oracle.de
> D-60311 Frankfurt am Main
>
> We make the net work
>
> NOTICE: This email message is for the sole use of the intended
> recipient(s) and may contain confidential and privileged information.
> Any unauthorized review, use, disclosure or distribution is prohibited.
> If you are not the intended recipient, please contact the sender by
> reply email and destroy all copies of the original message.
>
> Sitz der Gesellschaft:
> ORACLE Deutschland B.V. & Co. KG
> Hauptverwaltung: Riesstr. 25, D-80992 München
> Registergericht: Amtsgericht München, HRA 95603
> Geschäftsführer: Jürgen Kunz
>
> Komplementärin: ORACLE Deutschland Verwaltung B.V.
> Hertogswetering 163/167, 3543 AS Utrecht, Niederlande
> Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697
> Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher
>
> Dear customer, in order to help us serve you better,
> please request a survey after SR closure:
>
> https://oracle-support-surveys.custhelp.com/ci/documents/detail/5/280/12/131aae4624b2f151c23d3aed809e443c27ac405b
>
> Oracle is committed to developing practices and products that help
> protect the environment
>
> ___
> openconnect-devel mailing list
> openconnect-devel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/openconnect-devel
___
openconnect-devel mailing list
openconnect-devel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/openconnect-devel
