Re: 'struct ipoption' & signed char
here is a related argument: https://lists.freebsd.org/pipermail/svn-src-stable-9/2016-September/008613.html https://reviews.freebsd.org/D There is a minute C++ argument against changing it, but I suspect a library using ipopts is a rare animal.
Re: 'struct ipoption' & signed char
Alexander Bluhm wrote: > On Thu, Jan 16, 2020 at 02:35:25PM +0100, Mark Kettenis wrote: > > As for your suggestion. I think it is a bit of a mess. I wasn't > > entirely sure about the consequences of such a change for ports. > > However, "Stevens" has this as "char ipopt_list[]" which means that it > > was signed on some platforms and unsigned on others. Interestingly > > enough it has been int8_t since 1.1 in out tree... > > The only non-kernel user in our tree is lib/libc/rpc/svc_tcp.c. > There the value is always casted to u_char. The defines for this > field in netinet/ip.h are numbers also above 127. So char looks > like a historic mistake to we. We should fix it. This signed-type is not unique to openbsd, this is a very old definition. Have other systems changed it to an unsigned type, and avoided any unknown fallout? If we find one system which changed it, I think we're good. > Who expects fallout with IP options? We block them in pf per > default. don't make this about whether ip options are received, the argument is spurious since it's not like we can delete this structure ...
Re: 'struct ipoption' & signed char
On Thu, Jan 16, 2020 at 02:35:25PM +0100, Mark Kettenis wrote:
> As for your suggestion. I think it is a bit of a mess. I wasn't
> entirely sure about the consequences of such a change for ports.
> However, "Stevens" has this as "char ipopt_list[]" which means that it
> was signed on some platforms and unsigned on others. Interestingly
> enough it has been int8_t since 1.1 in out tree...
The only non-kernel user in our tree is lib/libc/rpc/svc_tcp.c.
There the value is always casted to u_char. The defines for this
field in netinet/ip.h are numbers also above 127. So char looks
like a historic mistake to we. We should fix it.
Who expects fallout with IP options? We block them in pf per
default.
bluhm
> > Index: netinet/ip_var.h
> > ===
> > RCS file: /cvs/src/sys/netinet/ip_var.h,v
> > retrieving revision 1.86
> > diff -u -p -r1.86 ip_var.h
> > --- netinet/ip_var.h8 Dec 2019 11:08:22 - 1.86
> > +++ netinet/ip_var.h16 Jan 2020 09:48:41 -
> > @@ -92,7 +92,7 @@ structipstat {
> >
> > struct ipoption {
> > struct in_addr ipopt_dst; /* first-hop dst if source routed */
> > - int8_t ipopt_list[MAX_IPOPTLEN]; /* options proper */
> > + uint8_t ipopt_list[MAX_IPOPTLEN]; /* options proper */
> > };
> >
> > #ifdef _KERNEL
> >
> >
Re: 'struct ipoption' & signed char
> Date: Thu, 16 Jan 2020 10:52:06 +0100
> From: Martin Pieuchot
>
> Found while compiling sgi kernel:
>
> /sys/netinet/igmp.c:140:22: error: implicit conversion from 'int' to 'int8_t'
> (aka 'signed char') changes value from 148 to -108
> [-Werror,-Wconstant-conversion]
> ra->ipopt_list[0] = IPOPT_RA;
> ~ ^~~~
> /sys/netinet/ip.h:153:19: note: expanded from macro 'IPOPT_RA'
>
> Use an unsigned type in this case:
We sidestepped this issue by adding -Wno-constant-conversion to
CWARNFLAGS for targets that were switched to clang. That happened for
octeon, but not for sgi and loongson. Something needs to be fixed
there (although we may simply give up on those platforms).
Actually since loongson is mips64el, it stall has gcc as the default
compiler, so the issue won't show up there (yet).
As for your suggestion. I think it is a bit of a mess. I wasn't
entirely sure about the consequences of such a change for ports.
However, "Stevens" has this as "char ipopt_list[]" which means that it
was signed on some platforms and unsigned on others. Interestingly
enough it has been int8_t since 1.1 in out tree...
> Index: netinet/ip_var.h
> ===
> RCS file: /cvs/src/sys/netinet/ip_var.h,v
> retrieving revision 1.86
> diff -u -p -r1.86 ip_var.h
> --- netinet/ip_var.h 8 Dec 2019 11:08:22 - 1.86
> +++ netinet/ip_var.h 16 Jan 2020 09:48:41 -
> @@ -92,7 +92,7 @@ struct ipstat {
>
> struct ipoption {
> struct in_addr ipopt_dst; /* first-hop dst if source routed */
> - int8_t ipopt_list[MAX_IPOPTLEN]; /* options proper */
> + uint8_t ipopt_list[MAX_IPOPTLEN]; /* options proper */
> };
>
> #ifdef _KERNEL
>
>
Re: 'struct ipoption' & signed char
On Thu, Jan 16, 2020 at 10:52:06AM +0100, Martin Pieuchot wrote:
> Found while compiling sgi kernel:
>
> /sys/netinet/igmp.c:140:22: error: implicit conversion from 'int' to 'int8_t'
> (aka 'signed char') changes value from 148 to -108
> [-Werror,-Wconstant-conversion]
> ra->ipopt_list[0] = IPOPT_RA;
> ~ ^~~~
> /sys/netinet/ip.h:153:19: note: expanded from macro 'IPOPT_RA'
>
> Use an unsigned type in this case:
People argued against some of these types of changes previously.
That is why kernels built with clang use -Wno-constant-conversion.
patrick proposed this change in 2017 but it didn't happen
https://marc.info/?l=openbsd-tech&m=148490401504785&w=2
>
> Index: netinet/ip_var.h
> ===
> RCS file: /cvs/src/sys/netinet/ip_var.h,v
> retrieving revision 1.86
> diff -u -p -r1.86 ip_var.h
> --- netinet/ip_var.h 8 Dec 2019 11:08:22 - 1.86
> +++ netinet/ip_var.h 16 Jan 2020 09:48:41 -
> @@ -92,7 +92,7 @@ struct ipstat {
>
> struct ipoption {
> struct in_addr ipopt_dst; /* first-hop dst if source routed */
> - int8_t ipopt_list[MAX_IPOPTLEN]; /* options proper */
> + uint8_t ipopt_list[MAX_IPOPTLEN]; /* options proper */
> };
>
> #ifdef _KERNEL
>
>
Re: 'struct ipoption' & signed char
On Thu, Jan 16, 2020 at 10:52:06AM +0100, Martin Pieuchot wrote:
> Found while compiling sgi kernel:
>
> /sys/netinet/igmp.c:140:22: error: implicit conversion from 'int' to 'int8_t'
> (aka 'signed char') changes value from 148 to -108
> [-Werror,-Wconstant-conversion]
> ra->ipopt_list[0] = IPOPT_RA;
> ~ ^~~~
> /sys/netinet/ip.h:153:19: note: expanded from macro 'IPOPT_RA'
>
> Use an unsigned type in this case:
OK bluhm@
> Index: netinet/ip_var.h
> ===
> RCS file: /cvs/src/sys/netinet/ip_var.h,v
> retrieving revision 1.86
> diff -u -p -r1.86 ip_var.h
> --- netinet/ip_var.h 8 Dec 2019 11:08:22 - 1.86
> +++ netinet/ip_var.h 16 Jan 2020 09:48:41 -
> @@ -92,7 +92,7 @@ struct ipstat {
>
> struct ipoption {
> struct in_addr ipopt_dst; /* first-hop dst if source routed */
> - int8_t ipopt_list[MAX_IPOPTLEN]; /* options proper */
> + uint8_t ipopt_list[MAX_IPOPTLEN]; /* options proper */
> };
>
> #ifdef _KERNEL
'struct ipoption' & signed char
Found while compiling sgi kernel:
/sys/netinet/igmp.c:140:22: error: implicit conversion from 'int' to 'int8_t'
(aka 'signed char') changes value from 148 to -108
[-Werror,-Wconstant-conversion]
ra->ipopt_list[0] = IPOPT_RA;
~ ^~~~
/sys/netinet/ip.h:153:19: note: expanded from macro 'IPOPT_RA'
Use an unsigned type in this case:
Index: netinet/ip_var.h
===
RCS file: /cvs/src/sys/netinet/ip_var.h,v
retrieving revision 1.86
diff -u -p -r1.86 ip_var.h
--- netinet/ip_var.h8 Dec 2019 11:08:22 - 1.86
+++ netinet/ip_var.h16 Jan 2020 09:48:41 -
@@ -92,7 +92,7 @@ structipstat {
struct ipoption {
struct in_addr ipopt_dst; /* first-hop dst if source routed */
- int8_t ipopt_list[MAX_IPOPTLEN]; /* options proper */
+ uint8_t ipopt_list[MAX_IPOPTLEN]; /* options proper */
};
#ifdef _KERNEL
