Re: CVS commit: src/external/bsd/dhcpcd/dist/src

2018-08-03 Thread Robert Elz
Date:Fri, 3 Aug 2018 02:17:33 + From:"Kamil Rytarowski" Message-ID: <20180803021733.b2002f...@cvs.netbsd.org> | GCC with -fsanitize=undefiend detects a potential overflow in the code. | Cast the return value of ntohs(3) to size_t. I don't understand the

Re: CVS commit: src/external/bsd/dhcpcd/dist/src

2018-08-03 Thread Kamil Rytarowski
On 03.08.2018 12:26, Robert Elz wrote: > Date:Fri, 3 Aug 2018 02:17:33 + > From:"Kamil Rytarowski" > Message-ID: <20180803021733.b2002f...@cvs.netbsd.org> > > | GCC with -fsanitize=undefiend detects a potential overflow in the code. > | Cast the return value

Re: CVS commit: src/external/bsd/dhcpcd/dist/src

2018-08-03 Thread Kamil Rytarowski
On 03.08.2018 15:20, Martin Husemann wrote: > On Fri, Aug 03, 2018 at 03:18:18PM +0200, Kamil Rytarowski wrote: >> The change was indicating to the compiler that code is safe, without >> changing the algorithm. > > I don't get why. > > Martin > Overflow (underflow) of an unsigned value is

Re: CVS commit: src/external/bsd/dhcpcd/dist/src

2018-08-03 Thread Martin Husemann
On Fri, Aug 03, 2018 at 03:18:18PM +0200, Kamil Rytarowski wrote: > The change was indicating to the compiler that code is safe, without > changing the algorithm. I don't get why. Martin

Re: CVS commit: src/external/bsd/dhcpcd/dist/src

2018-08-03 Thread Kamil Rytarowski
On 03.08.2018 15:02, Martin Husemann wrote: > On Fri, Aug 03, 2018 at 02:47:53PM +0200, Kamil Rytarowski wrote: >>> Further if there ever was a potential problem from this line ... >>> >>> *len = ntohs(p->ip.ip_len) - sizeof(p->ip) - sizeof(p->udp); >>> then >>> *len =

Re: CVS commit: src/external/bsd/dhcpcd/dist/src

2018-08-03 Thread Martin Husemann
On Fri, Aug 03, 2018 at 02:47:53PM +0200, Kamil Rytarowski wrote: > > Further if there ever was a potential problem from this line ... > > > > *len = ntohs(p->ip.ip_len) - sizeof(p->ip) - sizeof(p->udp); > > then > > *len = (size_t)ntohs(p->ip.ip_len) - sizeof(p->ip) - sizeof(p->udp); >

Re: CVS commit: src/external/bsd/dhcpcd/dist/src

2018-08-03 Thread Robert Elz
Date:Fri, 3 Aug 2018 15:02:28 +0200 From:Martin Husemann Message-ID: <20180803130227.ga23...@mail.duskware.de> | What exactly makes the code safe now? If ntohs(p->ip.ip_len) < | (sizeof(p->ip) + sizeof(p->udp)) then we are now in even more serious | trouble.

Re: CVS commit: src/external/bsd/dhcpcd/dist/src

2018-08-03 Thread Martin Husemann
On Fri, Aug 03, 2018 at 08:28:55PM +0700, Robert Elz wrote: > Where is the signed arithmetic that was supposedly a probem? Ah, stupid C integer promotion rules. uint16_t is promoted to int here, not unsigned int or size_t. The cast makes all operands the same type and no promotion happens.

Re: CVS commit: src/external/bsd/dhcpcd/dist/src

2018-08-03 Thread Robert Elz
Date:Fri, 3 Aug 2018 15:54:24 +0200 From:Martin Husemann Message-ID: <20180803135424.gc23...@mail.duskware.de> | Ah, stupid C integer promotion rules. uint16_t is promoted to int | here, not unsigned int or size_t. Even with that, there should be no problem, in

Re: CVS commit: src/external/bsd/dhcpcd/dist/src

2018-08-03 Thread Valery Ushakov
On Fri, Aug 03, 2018 at 15:54:24 +0200, Martin Husemann wrote: > On Fri, Aug 03, 2018 at 08:28:55PM +0700, Robert Elz wrote: > > Where is the signed arithmetic that was supposedly a probem? > > Ah, stupid C integer promotion rules. uint16_t is promoted to int > here, not unsigned int or size_t.

Re: CVS commit: src/external/bsd/dhcpcd/dist/src

2018-08-03 Thread Robert Elz
Date:Sat, 4 Aug 2018 02:15:15 +0200 From:Kamil Rytarowski Message-ID: | In general there shall not be a relation between -O level and | sanitizers. Sanitizers do not need -O0 or -g for operation. That's fine. But are you doing compiles that way? (necessary

Re: CVS commit: src/external/bsd/dhcpcd/dist/src

2018-08-03 Thread Valery Ushakov
On Fri, Aug 03, 2018 at 18:08:24 +0300, Valery Ushakov wrote: > On Fri, Aug 03, 2018 at 15:54:24 +0200, Martin Husemann wrote: > > > On Fri, Aug 03, 2018 at 08:28:55PM +0700, Robert Elz wrote: > > > Where is the signed arithmetic that was supposedly a probem? > > > > Ah, stupid C integer

Re: CVS commit: src/external/bsd/dhcpcd/dist/src

2018-08-03 Thread Kamil Rytarowski
On 04.08.2018 01:31, Robert Elz wrote: > Kamil: assuming you agree that this is a reasonable analysis, I'd suggest > no more code changes based upon gcc warnings issued this way. In general there shall not be a relation between -O level and sanitizers. Sanitizers do not need -O0 or -g for

Re: CVS commit: src/external/bsd/dhcpcd/dist/src

2018-08-03 Thread Roy Marples
On 03/08/2018 14:02, Martin Husemann wrote: On Fri, Aug 03, 2018 at 02:47:53PM +0200, Kamil Rytarowski wrote: Further if there ever was a potential problem from this line ... *len = ntohs(p->ip.ip_len) - sizeof(p->ip) - sizeof(p->udp); then *len = (size_t)ntohs(p->ip.ip_len) -

Re: CVS commit: src/external/bsd/dhcpcd/dist/src

2018-08-03 Thread Roy Marples
On 03/08/2018 15:22, Robert Elz wrote: Whether there need to be any attention to the possibility of a malformed packet I will leave for Roy to decide (I am assuming probably not) but that added cast just looks to be a bandaid for a broken compiler (sanitiser). The contents are verified further

Re: CVS commit: src/external/bsd/dhcpcd/dist/src

2018-08-03 Thread Kamil Rytarowski
On 03.08.2018 20:49, Roy Marples wrote: > On 03/08/2018 15:22, Robert Elz wrote: >> Whether there need to be any attention to the possibility >> of a malformed packet I will leave for Roy to decide (I am >> assuming probably not) but that added cast just looks to be >> a bandaid for a broken

Re: CVS commit: src/external/bsd/dhcpcd/dist/src

2018-08-03 Thread Robert Elz
Date:Fri, 3 Aug 2018 19:46:01 +0100 From:Roy Marples Message-ID: | Considering both methods work and the result of htons is a uint16_t (but | is this always guaranteed?) ntohs() (not that it matters) and "always" is a big word, but that is how it is defined by

Re: CVS commit: src/external/bsd/dhcpcd/dist/src

2018-08-03 Thread maya
On Fri, Aug 03, 2018 at 07:46:01PM +0100, Roy Marples wrote: > We could split the term, but merely storing the result of htons in it's own > variable creates a larger binary for no good reason as i see it. > I suspect that the compiler will generate the same code anyway when using a local

Re: CVS commit: src/external/bsd/dhcpcd/dist/src

2018-08-03 Thread Robert Elz
Date:Sat, 4 Aug 2018 04:24:01 +0200 From:Kamil Rytarowski Message-ID: <568544f4-36d5-853e-cdf8-248f84fad...@gmx.com> | I haven't changed any optimization or similar flags for the builds. Then why did the ssh example start giving "perhaps used uninit" warnings when

Re: CVS commit: src/external/bsd/dhcpcd/dist/src

2018-08-03 Thread Kamil Rytarowski
On 04.08.2018 03:23, Robert Elz wrote: > Date:Sat, 4 Aug 2018 02:15:15 +0200 > From:Kamil Rytarowski > Message-ID: > > > | In general there shall not be a relation between -O level and > | sanitizers. Sanitizers do not need -O0 or -g for operation. > > That's