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

2020-05-14 Thread Christos Zoulas
In article <95034282-ebf6-c1d5-8bb1-9258ee825...@marples.name>,
Roy Marples   wrote:
>On 10/05/2020 18:58, Christos Zoulas wrote:
>> Module Name: src
>> Committed By:christos
>> Date:Sun May 10 17:58:16 UTC 2020
>> 
>> Modified Files:
>>  src/external/bsd/dhcpcd/dist/src: dhcpcd.c
>> 
>> Log Message:
>> Add SIGPIPE to the list of dhcpcd affected signals since we sigignore it.
>
>Why?

Because the forked programs from scripts were executed with SIGPIPE blocked.
If that breaks non-kqueue OS's we should just add SIGPIPE to the signal
mask to default for posix_spawn().

christos



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

2020-05-14 Thread Roy Marples

On 10/05/2020 18:58, Christos Zoulas wrote:

Module Name:src
Committed By:   christos
Date:   Sun May 10 17:58:16 UTC 2020

Modified Files:
src/external/bsd/dhcpcd/dist/src: dhcpcd.c

Log Message:
Add SIGPIPE to the list of dhcpcd affected signals since we sigignore it.


Why?

BTW, this breaks non kqueue OS's so can't be accepted upstream.

Roy


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

2020-02-15 Thread Roy Marples

On 14/02/2020 20:36, Santhosh Raju wrote:

On Thu, Feb 13, 2020 at 4:44 PM Santhosh Raju  wrote:


On Thu, Feb 13, 2020 at 4:32 PM Kamil Rytarowski  wrote:


On 13.02.2020 22:20, Valery Ushakov wrote:

I did not propose to disable the warning.  I proposed to downgrade
-Werror to -Wno-error (i.e. a warning) and only for the buggy
sanitizer build.  That file will still be compiled in normal builds
with all the warnings=errors enabled, so real problems won't be
overlooked.


OK, we can try this path.

Santosh, could you please revert and try -Wno-error + upstream it?

Thank you in advance!



Sure, let me prepare the patch.



The patch has been prepared.

The builds were run both with and without MKLIBCSANITIZER=yes and it
was completed successfully.

Let us know if it alright to commit this.


Looks ok to me, commit it.

Roy


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

2020-02-14 Thread Santhosh Raju
On Thu, Feb 13, 2020 at 4:44 PM Santhosh Raju  wrote:
>
> On Thu, Feb 13, 2020 at 4:32 PM Kamil Rytarowski  wrote:
> >
> > On 13.02.2020 22:20, Valery Ushakov wrote:
> > > I did not propose to disable the warning.  I proposed to downgrade
> > > -Werror to -Wno-error (i.e. a warning) and only for the buggy
> > > sanitizer build.  That file will still be compiled in normal builds
> > > with all the warnings=errors enabled, so real problems won't be
> > > overlooked.
> >
> > OK, we can try this path.
> >
> > Santosh, could you please revert and try -Wno-error + upstream it?
> >
> > Thank you in advance!
> >
>
> Sure, let me prepare the patch.
>

The patch has been prepared.

The builds were run both with and without MKLIBCSANITIZER=yes and it
was completed successfully.

Let us know if it alright to commit this.

> --
> Santhosh

--
Santhosh
Index: dist/src/dhcp.c
===
RCS file: /cvsroot/src/external/bsd/dhcpcd/dist/src/dhcp.c,v
retrieving revision 1.33
diff -u -p -u -r1.33 dhcp.c
--- dist/src/dhcp.c	8 Feb 2020 12:17:16 -	1.33
+++ dist/src/dhcp.c	14 Feb 2020 20:32:20 -
@@ -3307,7 +3307,7 @@ is_packet_udp_bootp(void *packet, size_t
 	memcpy(, (char *)ip + ip_hlen, sizeof(udp));
 	if (ntohs(udp.uh_ulen) < sizeof(udp))
 		return false;
-	if (ip_hlen + (size_t)ntohs(udp.uh_ulen) > plen)
+	if (ip_hlen + ntohs(udp.uh_ulen) > plen)
 		return false;
 
 	/* Check it's to and from the right ports. */
Index: sbin/dhcpcd/Makefile
===
RCS file: /cvsroot/src/external/bsd/dhcpcd/sbin/dhcpcd/Makefile,v
retrieving revision 1.50
diff -u -p -u -r1.50 Makefile
--- sbin/dhcpcd/Makefile	29 Jan 2020 23:42:57 -	1.50
+++ sbin/dhcpcd/Makefile	14 Feb 2020 20:32:20 -
@@ -27,6 +27,11 @@ SRCS+=		auth.c
 .if (${USE_INET} != "no")
 CPPFLAGS+=	-DINET
 SRCS+=		bpf.c dhcp.c ipv4.c
+.if (${MKLIBCSANITIZER:Uno} == "yes")
+.if (${ACTIVE_CC} == "gcc" && ${HAVE_GCC:U0} == 8)
+COPTS.dhcp.c+=	-Wno-error=sign-conversion
+.endif
+.endif
 .if !defined(SMALLPROG)
 CPPFLAGS+=	-DARP
 SRCS+=		arp.c


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

2020-02-13 Thread Santhosh Raju
On Thu, Feb 13, 2020 at 4:32 PM Kamil Rytarowski  wrote:
>
> On 13.02.2020 22:20, Valery Ushakov wrote:
> > I did not propose to disable the warning.  I proposed to downgrade
> > -Werror to -Wno-error (i.e. a warning) and only for the buggy
> > sanitizer build.  That file will still be compiled in normal builds
> > with all the warnings=errors enabled, so real problems won't be
> > overlooked.
>
> OK, we can try this path.
>
> Santosh, could you please revert and try -Wno-error + upstream it?
>
> Thank you in advance!
>

Sure, let me prepare the patch.

--
Santhosh


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

2020-02-13 Thread Kamil Rytarowski
On 13.02.2020 22:20, Valery Ushakov wrote:
> I did not propose to disable the warning.  I proposed to downgrade
> -Werror to -Wno-error (i.e. a warning) and only for the buggy
> sanitizer build.  That file will still be compiled in normal builds
> with all the warnings=errors enabled, so real problems won't be
> overlooked.

OK, we can try this path.

Santosh, could you please revert and try -Wno-error + upstream it?

Thank you in advance!



signature.asc
Description: OpenPGP digital signature


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

2020-02-13 Thread Valery Ushakov
On Thu, Feb 13, 2020 at 18:25:41 +0100, Kamil Rytarowski wrote:

> On 13.02.2020 18:00, Valery Ushakov wrote:
> > Arguably, if the tool you use is broken, you shouln't be mutilating
> > the source code just to shut that tool up. 
> 
> The introduced changes were good, even if GCC would be silent.

You changed one occurence just because it happens to trigger a bug in
gcc.  There are ntohs() >< size_t_var comparisons right above and
below the line you changed, where the same promotion happens (without
triggering a gcc bug) and they don't have a cast.  So someone reading
that code will ask themself, wait a minute, why the cast is necessary
in *that* place but not in those places?  What is going on there that
I miss that required introduction of that cast.  I.e. your change
create cognitive load for the reader.  I don't cosider that good.


> [...] the promotion rules are considered by many people as the major
> flaw in C.  Today I prefer explicit casts (after the MUSL lesson)
> even if unnecessary than implicit promotion.

Amen.  However they are there and we made uneasy peace with them so
unless you are consistent in your casting, you send all the wrong
singals to the reader.


> As an alternative option we can disable warnings but this is in my
> opinion much worse in this case than potentially overlooking real
> problems in a 4000+ line file.

I did not propose to disable the warning.  I proposed to downgrade
-Werror to -Wno-error (i.e. a warning) and only for the buggy
sanitizer build.  That file will still be compiled in normal builds
with all the warnings=errors enabled, so real problems won't be
overlooked.


> Repeatedly informing that the tool (GCC) is broken did not solve any
> issue.  It would be finally better to see someone fixing GCC rather
> than informing other GCC users about it.

Feel free to follow your own advice here...  (After all, it's your
sanitizer usage that has problems.  The normal builds are fine as they
are :)


> On the opposite side of this if this camp is MUSL. I used chunks of the
> MUSL code and it had poor results. There is a strict policy to avoid
> casts unless absolutely needed

That's not a bad policy.  As I said, a cast is a blunt tool.  It's
easy to introduce a wrong one that would silence some warning or other
but will do the wrong thing, but now since there is a cast the
compiler cannot even squeak.  I might be misremembering, but IIRC
Visual C has some warning(s) that ignore explicit casts, precisely to
detect that kind of problems.


> and if they are needed this tends to be in as obscure way as
> possible (like adding U attribute to one of the arguments in an
> expression).

What's obscure about adding 'U' to signify unsignedness?!  Also it's
not a cast to begin with, a literal with the 'u' suffix *is* unsigned.
A cast is when you take something of one type and coerce it to be of
some other type.


-uwe


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

2020-02-13 Thread Kamil Rytarowski
On 13.02.2020 18:00, Valery Ushakov wrote:
> Arguably, if the tool you use is broken, you shouln't be mutilating
> the source code just to shut that tool up. 

The introduced changes were good, even if GCC would be silent. It is far
from mutilating. As an alternative option we can disable warnings but
this is in my opinion much worse in this case than potentially
overlooking real problems in a 4000+ line file.

On the opposite side of this if this camp is MUSL. I used chunks of the
MUSL code and it had poor results. There is a strict policy to avoid
casts unless absolutely needed and if they are needed this tends to be
in as obscure way as possible (like adding U attribute to one of the
arguments in an expression). This resulted in unmaintainable code and
very difficult situation to guess whether code semantics is broken or
buggy. For purists, MUSL is not mutilated at all so people wanting this
style are informed where to find it now.

Today I prefer explicit casts (after the MUSL lesson) even if
unnecessary than implicit promotion. I'm not alone in here as the
promotion rules are considered by many people as the major flaw in C.

Everybody agrees that GCC was picky in that case and not mandatory from
a strict language point of view.

Repeatedly informing that the tool (GCC) is broken did not solve any
issue. It would be finally better to see someone fixing GCC rather than
informing other GCC users about it.



signature.asc
Description: OpenPGP digital signature


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

2020-02-13 Thread Valery Ushakov
On Thu, Feb 13, 2020 at 15:03:35 +0100, Kamil Rytarowski wrote:

> On 13.02.2020 14:50, Valery Ushakov wrote:
> > On Thu, Feb 13, 2020 at 14:18:43 +0100, Kamil Rytarowski wrote:
> > 
> > Can you show the original problem that you are trying to fix here?
> > When and how did you get warning?
> 
> GCC has a property (as called by Joerg bug) that different backend,
> different optimization levels etc emit different warnings.
> 
> There is a request from certain people around to fix GCC not to mute
> down GCC, only because warnings are emitted in one configuration and not
> in their build settings.
> 
> I redirect these complains to GCC upstream.
> 
> dhcpcd emits these warnings once we enable -fsanitize=undefined.

If I add -fsanitize=undefined to the command line to compile that file
(in current) I still do not see that warning.  I didn't have time yet
to do a full build with sanitizer enabled.

Arguably, if the tool you use is broken, you shouln't be mutilating
the source code just to shut that tool up.  In general I'm quite
worried about the recent ongoing activity of sprinkling changes around
the tree just to shut up this or that warning without paying much
attention to the end result.  Some of those changes were wrong. Some
left code looking inconsistent as they fixed one place that did
produce a warning, but didn't change nearly identical bits of code
just next to it that didn't.  That leaves code in a fractured state.
Casts in particular are a very blunt instrument, and given that
history of recent changes your use of that instrument in this case
really raised red flags.

If -- and I still haven't seen it myself, "works for me" in a naive
test -- -fsanitize=undefined is buggy w.r.t. -Wconversion, then you
should arange to use say -Wno-error=conversion for that specific file
when -fsanitize=undefined is enabled, we do have makefile machinery
for that.  This way the bug in the tool is not causing random and
dubious changes to the code, and is confined to a properly commented
change in a Makefile.

-uwe


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

2020-02-13 Thread Kamil Rytarowski
On 13.02.2020 14:50, Valery Ushakov wrote:
> On Thu, Feb 13, 2020 at 14:18:43 +0100, Kamil Rytarowski wrote:
> 
>> 
> Can you show the original problem that you are trying to fix here?
> When and how did you get warning?
> 

GCC has a property (as called by Joerg bug) that different backend,
different optimization levels etc emit different warnings.

There is a request from certain people around to fix GCC not to mute
down GCC, only because warnings are emitted in one configuration and not
in their build settings.

I redirect these complains to GCC upstream.

dhcpcd emits these warnings once we enable -fsanitize=undefined.



signature.asc
Description: OpenPGP digital signature


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

2020-02-13 Thread Valery Ushakov
On Thu, Feb 13, 2020 at 14:18:43 +0100, Kamil Rytarowski wrote:

> Feel free to fix GCC.

That's sounds rather dismissive to me.

Can you show the original problem that you are trying to fix here?
When and how did you get warning?

I do NOT see the warning for this kind of conversion with either
current (gcc 8) or on netbsd-8 (gcc 5).

If I revert this change the file is compiled just fine.  E.g. with a
very current current build:

$ nbmake-sparc dhcp.o
#   compile  dhcpcd/dhcp.o
sparc--netbsdelf-gcc -O2 ... -Wconversion -Wsign-compare -c .../dhcp.c
$



> dhcpcd already uses this explicit cast style, e.g. here:
>
> static void *
> get_udp_data(void *packet, size_t *len)
> {
> const struct ip *ip = packet;
> size_t ip_hl = (size_t)ip->ip_hl * 4;
> char *p = packet;
> 
> p += ip_hl + sizeof(struct udphdr);
> *len = (size_t)ntohs(ip->ip_len) - sizeof(struct udphdr) - ip_hl;
> return p;
> }


Again, I don't get any warnings if I change that to

   ... /*(size_t)*/ntohs(ip->ip_len) ...

I also tried 

   ... /*(size_t)ntohs*/(ip->ip_len) ...

as a quick and dirty attempt to emulate little endian where ntohs is a
nop (I don't have an up-to-date little endian build handy) and, again,
there's no warning.

Oh, actually, my tooldir has accreted quite a number of old superh
compilers, so:

$ cat size.c
typedef unsigned short uint16_t;
typedef unsigned long size_t;
#define ntohs(x) (x)

size_t
foo(uint16_t ip_len)
{
return ntohs(ip_len) - sizeof(uint16_t);
}
$ for CC in $TOOLDIR/bin/shle--netbsdelf-gcc-[0-9]*; do echo $(basename $CC); 
$CC -Wall -Wextra -Wconversion -Wsign-compare -O2 -S size.c; done
shle--netbsdelf-gcc-4.5.4
shle--netbsdelf-gcc-4.8.3
shle--netbsdelf-gcc-4.8.4
shle--netbsdelf-gcc-4.8.5
shle--netbsdelf-gcc-5.4.0
shle--netbsdelf-gcc-5.5.0
shle--netbsdelf-gcc-6.4.0
shle--netbsdelf-gcc-6.5.0
shle--netbsdelf-gcc-7.4.0
$

-uwe


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

2020-02-13 Thread Kamil Rytarowski
On 13.02.2020 14:13, Joerg Sonnenberger wrote:
> On Thu, Feb 13, 2020 at 02:05:12PM +0100, Kamil Rytarowski wrote:
>> On 13.02.2020 00:58, Joerg Sonnenberger wrote:
>>> On Mon, Feb 10, 2020 at 04:45:35PM +, Roy Marples wrote:
 On 09/02/2020 19:21, Joerg Sonnenberger wrote:
> On Sat, Feb 08, 2020 at 12:17:16PM +, Santhosh Raju wrote:
>> Module Name: src
>> Committed By:fox
>> Date:Sat Feb  8 12:17:16 UTC 2020
>>
>> Modified Files:
>>  src/external/bsd/dhcpcd/dist/src: dhcp.c
>>
>> Log Message:
>> external/bsd/dhcpcd: Fix a -Wconversion warning.
>>
>> Type cast uint16_t to size_t to prevent implicit type conversion.
>
> Seriously? That should not warn and no cast should be used either.

 What fix would you recommend then?
>>>
>>> Disable the warning in GCC and fill an upstream PR against it. A
>>> conversion from uint16_t to size_t is value preserving by definition of
>>> the ISO C platform limits. It should never create a warning.
>>>
>>> Joerg
>>>
>>
>> If we disable warnings in our core software in non-trivial source file
>> for over 4000 lines we will mute more serious issues in a pretty
>> sensitive file. We fixed this warning in upstream dhcpcd.
> 
> The fix is IMO significantly worse as it actually creates technical
> debt. Frankly, I would go so far and say that this should be reverted.
> This warning seems to be pretty fundamentally broken by design if the
> analysis so far is correct.
> 
> Joerg
> 

Feel free to fix GCC.

dhcpcd already uses this explicit cast style, e.g. here:

static void *
get_udp_data(void *packet, size_t *len)
{
const struct ip *ip = packet;
size_t ip_hl = (size_t)ip->ip_hl * 4;
char *p = packet;

p += ip_hl + sizeof(struct udphdr);
*len = (size_t)ntohs(ip->ip_len) - sizeof(struct udphdr) - ip_hl;
return p;
}



signature.asc
Description: OpenPGP digital signature


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

2020-02-13 Thread Joerg Sonnenberger
On Thu, Feb 13, 2020 at 02:05:12PM +0100, Kamil Rytarowski wrote:
> On 13.02.2020 00:58, Joerg Sonnenberger wrote:
> > On Mon, Feb 10, 2020 at 04:45:35PM +, Roy Marples wrote:
> >> On 09/02/2020 19:21, Joerg Sonnenberger wrote:
> >>> On Sat, Feb 08, 2020 at 12:17:16PM +, Santhosh Raju wrote:
>  Module Name: src
>  Committed By:fox
>  Date:Sat Feb  8 12:17:16 UTC 2020
> 
>  Modified Files:
>   src/external/bsd/dhcpcd/dist/src: dhcp.c
> 
>  Log Message:
>  external/bsd/dhcpcd: Fix a -Wconversion warning.
> 
>  Type cast uint16_t to size_t to prevent implicit type conversion.
> >>>
> >>> Seriously? That should not warn and no cast should be used either.
> >>
> >> What fix would you recommend then?
> > 
> > Disable the warning in GCC and fill an upstream PR against it. A
> > conversion from uint16_t to size_t is value preserving by definition of
> > the ISO C platform limits. It should never create a warning.
> > 
> > Joerg
> > 
> 
> If we disable warnings in our core software in non-trivial source file
> for over 4000 lines we will mute more serious issues in a pretty
> sensitive file. We fixed this warning in upstream dhcpcd.

The fix is IMO significantly worse as it actually creates technical
debt. Frankly, I would go so far and say that this should be reverted.
This warning seems to be pretty fundamentally broken by design if the
analysis so far is correct.

Joerg


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

2020-02-13 Thread Kamil Rytarowski
On 13.02.2020 00:58, Joerg Sonnenberger wrote:
> On Mon, Feb 10, 2020 at 04:45:35PM +, Roy Marples wrote:
>> On 09/02/2020 19:21, Joerg Sonnenberger wrote:
>>> On Sat, Feb 08, 2020 at 12:17:16PM +, Santhosh Raju wrote:
 Module Name:   src
 Committed By:  fox
 Date:  Sat Feb  8 12:17:16 UTC 2020

 Modified Files:
src/external/bsd/dhcpcd/dist/src: dhcp.c

 Log Message:
 external/bsd/dhcpcd: Fix a -Wconversion warning.

 Type cast uint16_t to size_t to prevent implicit type conversion.
>>>
>>> Seriously? That should not warn and no cast should be used either.
>>
>> What fix would you recommend then?
> 
> Disable the warning in GCC and fill an upstream PR against it. A
> conversion from uint16_t to size_t is value preserving by definition of
> the ISO C platform limits. It should never create a warning.
> 
> Joerg
> 

If we disable warnings in our core software in non-trivial source file
for over 4000 lines we will mute more serious issues in a pretty
sensitive file. We fixed this warning in upstream dhcpcd.

We ship with GCC that is typically 2 major versions behind upstream GCC
and this at least discourages handling generic issues directly with
upstream and upstream has little interest in reiterating over old
branches for corner cases that are generally speaking unimportant.

So overall I disagree with the request. Feel free to contribute a patch
to GCC or find someone in GCC to fix it (we have no resources to do it
on our side) so we can remove the cast.



signature.asc
Description: OpenPGP digital signature


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

2020-02-12 Thread Robert Elz
Date:Thu, 13 Feb 2020 02:45:44 +
From:Roy Marples 
Message-ID:  

  | My understanding was if it could be promoted to an int it would be.
  | So it size_t is bigger in bits than uint16_t and int is also bigger then 
  | promotion occurs and we then have signed vs unsigned.

You're right, but I think Joerg is even more right - or I certainly hope so.

The reasoning is that when the uint16_t gets promoted to int, since int
has more bits than the uint16_t the conversion is done by filling the
low 16 bits of the int with the uint16_t value, and zero filling the rest
(that is, value preserving).   You then have a signed int.  But it is
known to be >= 0.   When that is converted to a size_t (unsigned, and
here assumed to be at least as many bits as an int .. similar reasoning
applies if not) the signed int is sign extended to fit the size_t, and then
made unsigned.   "Sign exiended" here means zero fill, as we know the
value must be >= 0 (so the sign bit must be 0).

The only slightly weird case would be if int were 16 bits, but in that case,
a uint16_t would be an unsigned int, and no promotion to int ever happens,
it simply gets promoted directly to size_t.

If I had to guess, I'd say that gcc is losing the "must be >= 0" part of
the conversion from uint16_t to int, and assuming that the int might be
negative, which would be bad for converting to a size_t.

kre



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

2020-02-12 Thread Roy Marples

On 13/02/2020 02:17, Joerg Sonnenberger wrote:

I thought this fell under int promotion and thus became signed vs unsigned?


size_t is guaranteed to be at least 16bit. If INT_MAX == 32767, an
implicit cast of uint16_t would go to unsigned anyway and in all other
cases, any implicit cast must be value preserving.


My understanding was if it could be promoted to an int it would be.
So it size_t is bigger in bits than uint16_t and int is also bigger then 
promotion occurs and we then have signed vs unsigned.


Roy


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

2020-02-12 Thread Joerg Sonnenberger
On Thu, Feb 13, 2020 at 02:07:23AM +, Roy Marples wrote:
> On 12/02/2020 23:58, Joerg Sonnenberger wrote:
> > On Mon, Feb 10, 2020 at 04:45:35PM +, Roy Marples wrote:
> > > On 09/02/2020 19:21, Joerg Sonnenberger wrote:
> > > > On Sat, Feb 08, 2020 at 12:17:16PM +, Santhosh Raju wrote:
> > > > > Module Name:  src
> > > > > Committed By: fox
> > > > > Date: Sat Feb  8 12:17:16 UTC 2020
> > > > > 
> > > > > Modified Files:
> > > > >   src/external/bsd/dhcpcd/dist/src: dhcp.c
> > > > > 
> > > > > Log Message:
> > > > > external/bsd/dhcpcd: Fix a -Wconversion warning.
> > > > > 
> > > > > Type cast uint16_t to size_t to prevent implicit type conversion.
> > > > 
> > > > Seriously? That should not warn and no cast should be used either.
> > > 
> > > What fix would you recommend then?
> > 
> > Disable the warning in GCC and fill an upstream PR against it. A
> > conversion from uint16_t to size_t is value preserving by definition of
> > the ISO C platform limits. It should never create a warning.
> 
> I thought this fell under int promotion and thus became signed vs unsigned?

size_t is guaranteed to be at least 16bit. If INT_MAX == 32767, an
implicit cast of uint16_t would go to unsigned anyway and in all other
cases, any implicit cast must be value preserving.

Joerg


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

2020-02-12 Thread Roy Marples

On 12/02/2020 23:58, Joerg Sonnenberger wrote:

On Mon, Feb 10, 2020 at 04:45:35PM +, Roy Marples wrote:

On 09/02/2020 19:21, Joerg Sonnenberger wrote:

On Sat, Feb 08, 2020 at 12:17:16PM +, Santhosh Raju wrote:

Module Name:src
Committed By:   fox
Date:   Sat Feb  8 12:17:16 UTC 2020

Modified Files:
src/external/bsd/dhcpcd/dist/src: dhcp.c

Log Message:
external/bsd/dhcpcd: Fix a -Wconversion warning.

Type cast uint16_t to size_t to prevent implicit type conversion.


Seriously? That should not warn and no cast should be used either.


What fix would you recommend then?


Disable the warning in GCC and fill an upstream PR against it. A
conversion from uint16_t to size_t is value preserving by definition of
the ISO C platform limits. It should never create a warning.


I thought this fell under int promotion and thus became signed vs unsigned?

Roy


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

2020-02-12 Thread Joerg Sonnenberger
On Mon, Feb 10, 2020 at 04:45:35PM +, Roy Marples wrote:
> On 09/02/2020 19:21, Joerg Sonnenberger wrote:
> > On Sat, Feb 08, 2020 at 12:17:16PM +, Santhosh Raju wrote:
> > > Module Name:  src
> > > Committed By: fox
> > > Date: Sat Feb  8 12:17:16 UTC 2020
> > > 
> > > Modified Files:
> > >   src/external/bsd/dhcpcd/dist/src: dhcp.c
> > > 
> > > Log Message:
> > > external/bsd/dhcpcd: Fix a -Wconversion warning.
> > > 
> > > Type cast uint16_t to size_t to prevent implicit type conversion.
> > 
> > Seriously? That should not warn and no cast should be used either.
> 
> What fix would you recommend then?

Disable the warning in GCC and fill an upstream PR against it. A
conversion from uint16_t to size_t is value preserving by definition of
the ISO C platform limits. It should never create a warning.

Joerg


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

2020-02-10 Thread Roy Marples

On 09/02/2020 19:21, Joerg Sonnenberger wrote:

On Sat, Feb 08, 2020 at 12:17:16PM +, Santhosh Raju wrote:

Module Name:src
Committed By:   fox
Date:   Sat Feb  8 12:17:16 UTC 2020

Modified Files:
src/external/bsd/dhcpcd/dist/src: dhcp.c

Log Message:
external/bsd/dhcpcd: Fix a -Wconversion warning.

Type cast uint16_t to size_t to prevent implicit type conversion.


Seriously? That should not warn and no cast should be used either.


What fix would you recommend then?

Roy


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

2020-02-09 Thread Joerg Sonnenberger
On Sat, Feb 08, 2020 at 12:17:16PM +, Santhosh Raju wrote:
> Module Name:  src
> Committed By: fox
> Date: Sat Feb  8 12:17:16 UTC 2020
> 
> Modified Files:
>   src/external/bsd/dhcpcd/dist/src: dhcp.c
> 
> Log Message:
> external/bsd/dhcpcd: Fix a -Wconversion warning.
> 
> Type cast uint16_t to size_t to prevent implicit type conversion.

Seriously? That should not warn and no cast should be used either.

Joerg


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

2020-01-27 Thread Christos Zoulas
Thanks. Yes, I have the core-dump, no we should not remove the line...

Best,

christos

> On Jan 27, 2020, at 8:03 AM, Roy Marples  wrote:
> 
> On 27/01/2020 09:03, Roy Marples wrote:
>> On 26/01/2020 22:57, Christos Zoulas wrote:
>>> Module Name:src
>>> Committed By:christos
>>> Date:Sun Jan 26 22:57:52 UTC 2020
>>> 
>>> Modified Files:
>>> src/external/bsd/dhcpcd/dist/src: script.c
>>> 
>>> Log Message:
>>> prevent coredump when state == NULL
>>> 
>>> 
>>> To generate a diff of this commit:
>>> cvs rdiff -u -r1.1.1.12 -r1.2 src/external/bsd/dhcpcd/dist/src/script.c
>> A quick perusal through the code shows that this should not happen. Every 
>> time IPV4LL is given as a reason, the state should exist.
> 
> There is one code path where there may be no state  when an IPv4LL 
> address exists and dhcpcd no longer controlls it (ie dhcpcd restarted or a 
> 3rd party added it) then we delete it anyway (for say when we obtain a DHCP 
> address).
> We should still run the IPV4LL hook script, but it will be empty.
> 
> I've accepted this patch upstream.
> 
> Roy



signature.asc
Description: Message signed with OpenPGP


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

2020-01-27 Thread Roy Marples

On 27/01/2020 09:03, Roy Marples wrote:

On 26/01/2020 22:57, Christos Zoulas wrote:

Module Name:    src
Committed By:    christos
Date:    Sun Jan 26 22:57:52 UTC 2020

Modified Files:
src/external/bsd/dhcpcd/dist/src: script.c

Log Message:
prevent coredump when state == NULL


To generate a diff of this commit:
cvs rdiff -u -r1.1.1.12 -r1.2 src/external/bsd/dhcpcd/dist/src/script.c


A quick perusal through the code shows that this should not happen. Every time 
IPV4LL is given as a reason, the state should exist.


There is one code path where there may be no state  when an IPv4LL address 
exists and dhcpcd no longer controlls it (ie dhcpcd restarted or a 3rd party 
added it) then we delete it anyway (for say when we obtain a DHCP address).

We should still run the IPV4LL hook script, but it will be empty.

I've accepted this patch upstream.

Roy


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

2020-01-27 Thread Roy Marples

On 26/01/2020 22:57, Christos Zoulas wrote:

Module Name:src
Committed By:   christos
Date:   Sun Jan 26 22:57:52 UTC 2020

Modified Files:
src/external/bsd/dhcpcd/dist/src: script.c

Log Message:
prevent coredump when state == NULL


To generate a diff of this commit:
cvs rdiff -u -r1.1.1.12 -r1.2 src/external/bsd/dhcpcd/dist/src/script.c


A quick perusal through the code shows that this should not happen. Every time 
IPV4LL is given as a reason, the state should exist.
If there really is no state then the script should not be called at all - we 
wouldn't want to pass the error down to dhcpcd listeners who would expect either 
an old address or new address to be present.


Hopefully you still have the coredump so this can be analysed more?

Should we also remove this line fromm 3RDPARTY?
http://anonhg.netbsd.org/src/file/tip/doc/3RDPARTY#l340

Roy


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

2018-08-04 Thread Kamil Rytarowski
On 04.08.2018 05:44, Robert Elz wrote:
> 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 it had not before?Something changed.

I can refer to the opinion of Joerg (expressed on the NetBSD mailing
lists) that reporting uninitiated variables are one of the biggest bugs
in GCC.

I will leave tracking it down to someone with knowledge of GCC code
generation algorithms, and push a patch to OpenSSH.



signature.asc
Description: OpenPGP digital signature


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 it had not before?Something changed.

  | Freshly crashed pmax kernel due to integer overflow is a kernel (or
  | virtualization) bug, but it's also a definition of UB, that it can crash
  | the computer.

I don't know the circumstances of that one, but sure, overflow can cause
all kinds of problems, and if it actually occurs, almost anything is possible,
depending upon just what the code is doing.   I'm not sure what your point
is here, no-one is suggesting that real bugs not get fixed.

  | The dhcpcd one was fixed first in upstream dhcpcd before landing it to src/.

"fixed" is the wrong word, as there neither is, nor ever was, any bug there
to "fix".   You apparently silenced a bogus sanitiser induced warning.   That
is not a "fix".

kre



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 fine.   But are you doing compiles that way?  (necessary or not)
> 

I haven't changed any optimization or similar flags for the builds.

>   | GCC also enables more warnings for UBSan that have to be addressed in
>   | order to compile the source, as the code would be UB anyway (like
>   | changing the signedness bit with a shift operation).
> 
> Sure, some of those, even though they're not really problems, are easy
> to fix in a totally harmless way.   That's fine.   The UB is a technical C
> issue, not really anything that ever fails in those cases though.
> 

Freshly crashed pmax kernel due to integer overflow is a kernel (or
virtualization) bug, but it's also a definition of UB, that it can crash
the computer.

>   | And regarding utility of the Undefined Behavior Sanitizer and coverage
>   | of new tests.. we have just caught a bug on pmax that an integer
>   | overflow crashed the kernel:
> 
> Sure, no-one is saying that the extra work is not worth while.   Just that
> you are sometimes fixing non-problems (and causing code churn to do
> it - particularly when the code being changed comes from upstream ... in
> the dhcpcd case that is not such an issue, as if needed, Roy will fix that
> as well, but why would anyone expect the openbsd people to alter ssh
> to fix a non problem ?)
> 

Some Undefined Behavior fixes were pulled by FreeBSD (at least one of
them merged McKusick!). Regarding the OpenSSH case, I'm in touch with
some of their developers and concluded to push it to one of their
mailinglist. One of the UB patches was already merged by the OpenBSD
project (in tmux).

I will go the same way with other patches and submit them upstream, even
if some are cautious. The dhcpcd one was fixed first in upstream dhcpcd
before landing it to src/.



signature.asc
Description: OpenPGP digital signature


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 or not)

  | GCC is known for reporting uninitialized variables and I wouldn't blame
  | sanitizers for it.

I wasn't.   The one (which isn't) in the ssh code has been there for
ages, and has compiled successfully, for ages   There has to be a
reason why it only needed action yesterday.

  | We just initialize them to tune it down and this is the current practice.

Yes, I know - some of them are real potential problems, even if the
circumstances that lead to the problem are so unlikely that we never
see them in real life - others take knowledge about the environment
the compiler does not have, or just require flow analysis more
complex than it is reasonable to expect of the compiler, in order to know
that there is not actually a problem.The real bugs we fix, obviously.
The ones the compiler cannot detect are not bugs we deal with as you
said.   But when the compiler is able to detect there is no problem, but
we are simply preventing it doing so, we fix the way we use the compiler.

  | GCC also enables more warnings for UBSan that have to be addressed in
  | order to compile the source, as the code would be UB anyway (like
  | changing the signedness bit with a shift operation).

Sure, some of those, even though they're not really problems, are easy
to fix in a totally harmless way.   That's fine.   The UB is a technical C
issue, not really anything that ever fails in those cases though.

There has (is currently) an issue with posix, where a current bug resulution
requires abs(INT_MIN) to be INT_MIN (ie: abs(n) can end up < 0).   In C
that's undefined.   POSIX requires 2's complement however (unlike C) and
can rely upon what happens with 0 - INT_MAX even if C says that is
undefined.Some people like it, others do not...  (what the end result
will be I have no idea.)

  | I don't agree with strong opinions against cautious warnings/errors from
  | a compiler.

Sure, but when the warning goes off, we need to analyse the issue and
see what the actual problem is, not just blindly do whatever makes the
compiler stop issuing the warning.

  |They are there for purpose and dhcpcd could be really broken
  | with the same code, but with a different context.

No, it could not.   The only possible issue was if the packet was
invalid (too short a len) but that was not what the warning was
about (and could not have been, as there was nothing undefined
if that happened, just an unwanted result).

  | And regarding utility of the Undefined Behavior Sanitizer and coverage
  | of new tests.. we have just caught a bug on pmax that an integer
  | overflow crashed the kernel:

Sure, no-one is saying that the extra work is not worth while.   Just that
you are sometimes fixing non-problems (and causing code churn to do
it - particularly when the code being changed comes from upstream ... in
the dhcpcd case that is not such an issue, as if needed, Roy will fix that
as well, but why would anyone expect the openbsd people to alter ssh
to fix a non problem ?)

Once again, please do not change code to fix gcc warnings unless you
get the same warning with the code compiled with -O2 (or more).

kre



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 operation. UBSan does
not need disabled optimization for reporting issues in exact location in
the code. It also does not need debug information (DWARF or similar)...
however a runtime might make use of the additional data to print more
verbose messages or stacktraces.

GCC is known for reporting uninitialized variables and I wouldn't blame
sanitizers for it. We just initialize them to tune it down and this is
the current practice.

GCC also enables more warnings for UBSan that have to be addressed in
order to compile the source, as the code would be UB anyway (like
changing the signedness bit with a shift operation).

I don't agree with strong opinions against cautious warnings/errors from
a compiler. They are there for purpose and dhcpcd could be really broken
with the same code, but with a different context.

And regarding utility of the Undefined Behavior Sanitizer and coverage
of new tests.. we have just caught a bug on pmax that an integer
overflow crashed the kernel:

UB caused to crash pmax.. divrem_overflow_signed_div: pexpect reported
EOF - VMM exited unexpectedly



signature.asc
Description: OpenPGP digital signature


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 promotion rules.  uint16_t is promoted to int
> > here, not unsigned int or size_t.
> 
> Hmm, i don't think that's true.

Nah, you are right.  "THEN" is the important word here.

But as it transpired in another branch of this thread the problem was
something entirely different...

>   6.3.1.8  Usual arithmetic conversions
> 
>   ...
> Otherwise, the integer promotions are performed on both
> operands.   Then the following rules are applied to the
> promoted operands:

-uwe


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 variable for intermediate results, feel free to write
the cost as you find most legible :-)


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 POSIX, so it should be something that we can rely upon.

  | is this just an over-zealous compiler warning?

Not so much over zealous, as just plain lazy... (given Kamil's most
recent message):

  | conversion to 'long unsigned int' from 'int' may change the sign of the
  | result [-Werror=sign-conversion]
  |   *len = ntohs(p->ip.ip_len) - sizeof(p->ip) - sizeof(p->udp);
  |  ^

That's not the overflow on subtract that you said before, and for this one
I can see how the cast can help ... but that's just sheer laziness on the part
of the compiler.

The "int" which is there was created by the compiler, it knows (or should know)
that the underlying value is in the rangs [0..65535] and cannot possibly have
its sign changed when it is converted to long unsigned int.

It would be more understandable if the int appeared somewhere earlier, but
here this is all in this one expression, one type promotion on top of another.

Get the idiot compiler fixed, and then remove the cast.   In the meantime,
at least mark it with a comment indicating that the cast should not be needed,
and is there purely to appease gcc.

kre



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 compiler (sanitiser).
> 
> The contents are verified further up the stack.
> I'm inclined to agree it's a dodgy compiler warning, but I'm not really
> an expert here.
> 
> Roy

I've repeated the compiler error as I forgot the exact error message (it
was fixed in a local branch a while ago):

/public/src.git/external/bsd/dhcpcd/dist/src/dhcp.c: In function
'get_udp_data':
/public/src.git/external/bsd/dhcpcd/dist/src/dhcp.c:3270:29: error:
conversion to 'long unsigned int' from 'int' may change the sign of the
result [-Werror=sign-conversion]
  *len = ntohs(p->ip.ip_len) - sizeof(p->ip) - sizeof(p->udp);
 ^


http://netbsd.org/~kamil/patch-00068-dhcpcd-get_udp_data.txt



signature.asc
Description: OpenPGP digital signature


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 up the stack.
I'm inclined to agree it's a dodgy compiler warning, but I'm not really 
an expert here.


Roy


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) - sizeof(p->ip) - sizeof(p->udp);



It was a build time error generated by GCC. The compiler detected that
both sizeof() could be large enough to overflow the result returned from
  ntohs(3). And overflowing a signed integer is Undefined Behavior.


But we do not do this here.


This change points to the compiler that the code is safe.


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.

Does splitting the term help?

uint16_t hdr_size = sizeof(p->ip) - sizeof(p->udp);
uint16_t pkt_size = ntohs(p->ip.ip_len);
KASSERT(pkt_size > hdr_size);
*len = pkt_size > hdr_size ? pkt_size-hdr_size : 0;

or something like that?


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.


Considering both methods work and the result of htons is a uint16_t (but 
is this always guaranteed?) is this just an over-zealous compiler warning?


Roy


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.

Hmm, i don't think that's true.

  6.3.1.8  Usual arithmetic conversions

  ...
Otherwise, the integer promotions are performed on both
operands.   Then the following rules are applied to the
promoted operands:

 If both operands  have  the  same  type,  then  no
 further conversion is needed.

 Otherwise,  if  both  operands have signed integer
 types or both have  unsigned  integer  types,  the
 operand with the type of lesser integer conversion
 rank is converted to the type of the operand  with
 greater rank.

ntohs returns unsigned uint16_t, sizeof returns unsigned size_t, so
uint16_t, that has "lesser integer convertion rank" (i.e. it's
smaller) should be converted to size_t automagically.

-uwe


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

signed - unsigned

the '-' should be an unsigned - and the result should
be unsigned.   There is no signed arithmetic being done
here to cause an undefined result.

That's the same rule that makes

strlen(s) + 1

be a size_t rather than a ssize_t or whatever.   Otherwise we'd
need to be adding casts to every operation like that, just in case
strlen(s) == MAX_INT and the " +1 " would cause overflow, and
undefined operation.No thanks.

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).

kre



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.

Martin


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.

Actually, not more serious, the same serious as before.   If adding that
cast change anything at all, the compiler isn't working as it should.

If the values haven't been verieied, they should be.   If they have been
verified, there is no problem and nothing needs fixing (except possibly the
santiizer).

In a later message ...

  | Overflow (underflow) of an unsigned value is defined and GCC stops
  | deducing whether there might be a problem.

But it always was unsigned, ntohs() returns an unsigned result.   Further
even if it was signed, doesn't combining a signed value and an unsigned
one with an arithmetic op result in an unsigned operation?

Where is the signed arithmetic that was supposedly a probem?

kre




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 defined and GCC stops
deducing whether there might be a problem.



signature.asc
Description: OpenPGP digital signature


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 = (size_t)ntohs(p->ip.ip_len) - sizeof(p->ip) - sizeof(p->udp);
> 
>> It was a build time error generated by GCC. The compiler detected that
>> both sizeof() could be large enough to overflow the result returned from
>>  ntohs(3). And overflowing a signed integer is Undefined Behavior.
> 
> But we do not do this here.
> 
>> This change points to the compiler that the code is safe.
> 
> 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.
> 

The change was indicating to the compiler that code is safe, without
changing the algorithm.

> Does splitting the term help?
> 
>   uint16_t hdr_size = sizeof(p->ip) - sizeof(p->udp);

+

>   uint16_t pkt_size = ntohs(p->ip.ip_len);
>   KASSERT(pkt_size > hdr_size);
>   *len = pkt_size > hdr_size ? pkt_size-hdr_size : 0;
> 
> or something like that?
> 

This looks like a safer approach, but I will defer it to Roy to decide
what to do.

Previously we have agreed with the (size_t) case.

> Martin
> 




signature.asc
Description: OpenPGP digital signature


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);

> It was a build time error generated by GCC. The compiler detected that
> both sizeof() could be large enough to overflow the result returned from
>  ntohs(3). And overflowing a signed integer is Undefined Behavior.

But we do not do this here.

> This change points to the compiler that the code is safe.

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.

Does splitting the term help?

uint16_t hdr_size = sizeof(p->ip) - sizeof(p->udp);
uint16_t pkt_size = ntohs(p->ip.ip_len);
KASSERT(pkt_size > hdr_size);
*len = pkt_size > hdr_size ? pkt_size-hdr_size : 0;

or something like that?

Martin


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 of ntohs(3) to size_t.
> 
> I don't understand the point of this change, and I believe it to
> be wrong and misguided.
> 
> 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);
> 
> will not fix it.   The possible problem being if ip_len < 28 (the sum of the
> two sizeof's).While I didn't check this code, that possibility is usually
> verified elsewhere, and even if not, adding the cast changes nothing, the
> result is still bogus (that is, at best, the change could be masking a real
> bug, though that's unlikely.)
> 
> This looks to be (somehow) determining that the result of ntohs(ip_len)
> might somehow be negative (or something, I'm not really sure what is
> being believed is happening) but the ntohs() result is (in all cases here)
> a uint16_t (either because the result is literally the arg, which is that 
> type,
> or because it is the result of bswap16() which is declared that way.)
> 
> The sizeof() ops make the expression at least a size_t - so unless size_t
> and uint16_t are the same (which might happen on a pdp-11 or similar,
> though even there it is unlikely I suspect) the narrower one needs to be
> promoted - whether the ntohs() result is implicitly promoted to size_t, or
> explicitly cast cannot make any difference, can it?
> 
> So what exactly is being fixed here?  Which behaviour is supposedly undefined?
> 

It was a build time error generated by GCC. The compiler detected that
both sizeof() could be large enough to overflow the result returned from
 ntohs(3). And overflowing a signed integer is Undefined Behavior.

This change points to the compiler that the code is safe.

> kre
> 




signature.asc
Description: OpenPGP digital signature


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 point of this change, and I believe it to
be wrong and misguided.

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);

will not fix it.   The possible problem being if ip_len < 28 (the sum of the
two sizeof's).While I didn't check this code, that possibility is usually
verified elsewhere, and even if not, adding the cast changes nothing, the
result is still bogus (that is, at best, the change could be masking a real
bug, though that's unlikely.)

This looks to be (somehow) determining that the result of ntohs(ip_len)
might somehow be negative (or something, I'm not really sure what is
being believed is happening) but the ntohs() result is (in all cases here)
a uint16_t (either because the result is literally the arg, which is that type,
or because it is the result of bswap16() which is declared that way.)

The sizeof() ops make the expression at least a size_t - so unless size_t
and uint16_t are the same (which might happen on a pdp-11 or similar,
though even there it is unlikely I suspect) the narrower one needs to be
promoted - whether the ntohs() result is implicitly promoted to size_t, or
explicitly cast cannot make any difference, can it?

So what exactly is being fixed here?  Which behaviour is supposedly undefined?

kre