Re: EV_SET(2) shadows variable
On Sat, 4 Apr 2020, Theo de Raadt wrote:
> Philip Guenther wrote:
>
> > On Fri, 3 Apr 2020, Martin Pieuchot wrote:
> > > Thanks, here it is, ok?
> >
> > ok guenther@
>
> Should we do the same to all other macros, just in case?
Checking /usr/include/{,sys/}*.h, the diff below fixes the only ones I
found to be potential problems
/usr/include/net* and some others have not-completely-safe macros, like
IP6_EXTHDR_GET()
Index: include/bitstring.h
===
RCS file: /data/src/openbsd/src/include/bitstring.h,v
retrieving revision 1.5
diff -u -p -r1.5 bitstring.h
--- include/bitstring.h 2 Jun 2003 19:34:12 - 1.5
+++ include/bitstring.h 6 Apr 2020 00:37:52 -
@@ -83,46 +83,46 @@ typedef unsigned char bitstr_t;
/* clear bits start ... stop in bitstring */
#definebit_nclear(name, start, stop) do { \
- register bitstr_t *_name = name; \
- register int _start = start, _stop = stop; \
- while (_start <= _stop) { \
- bit_clear(_name, _start); \
- _start++; \
+ register bitstr_t *__name = (name); \
+ register int ___start = (start), __stop = (stop); \
+ while (__start <= __stop) { \
+ bit_clear(__name, __start); \
+ __start++; \
} \
} while(0)
/* set bits start ... stop in bitstring */
#definebit_nset(name, start, stop) do { \
- register bitstr_t *_name = name; \
- register int _start = start, _stop = stop; \
- while (_start <= _stop) { \
- bit_set(_name, _start); \
- _start++; \
+ register bitstr_t *__name = (name); \
+ register int __start = (start), __stop = (stop); \
+ while (__start <= __stop) { \
+ bit_set(__name, __start); \
+ __start++; \
} \
} while(0)
/* find first bit clear in name */
#definebit_ffc(name, nbits, value) do { \
- register bitstr_t *_name = name; \
- register int _bit, _nbits = nbits, _value = -1; \
- for (_bit = 0; _bit < _nbits; ++_bit) \
- if (!bit_test(_name, _bit)) { \
- _value = _bit; \
+ register bitstr_t *__name = (name); \
+ register int __bit, __nbits = (nbits), __value = -1; \
+ for (__bit = 0; __bit < __nbits; ++__bit) \
+ if (!bit_test(__name, __bit)) { \
+ __value = __bit; \
break; \
} \
- *(value) = _value; \
+ *(value) = __value; \
} while(0)
/* find first bit set in name */
#definebit_ffs(name, nbits, value) do { \
- register bitstr_t *_name = name; \
- register int _bit, _nbits = nbits, _value = -1; \
- for (_bit = 0; _bit < _nbits; ++_bit) \
- if (bit_test(_name, _bit)) { \
- _value = _bit; \
+ register bitstr_t *__name = (name); \
+ register int __bit, __nbits = (nbits), __value = -1; \
+ for (__bit = 0; __bit < __nbits; ++__bit) \
+ if (bit_test(__name, __bit)) { \
+ __value = __bit; \
break; \
} \
- *(value) = _value; \
+ *(value) = __value; \
} while(0)
#endif /* !_BITSTRING_H_ */
Index: sys/sys/disklabel.h
===
RCS file: /data/src/openbsd/src/sys/sys/disklabel.h,v
retrieving revision 1.75
diff -u -p -r1.75 disklabel.h
--- sys/sys/disklabel.h 24 Oct 2017 09:36:13 - 1.75
+++ sys/sys/disklabel.h 6 Apr 2020 00:52:08 -
@@ -156,37 +156,37 @@ struct__partitionv0 { /* old (v0) part
#define DL_GETPSIZE(p) (((u_int64_t)(p)->p_sizeh << 32) + (p)->p_size)
#define DL_SETPSIZE(p, n) do { \
- u_int64_t x = (n); \
- (p)->p_sizeh = x >> 32; \
- (p)->p_size = x; \
+ u_int64_t __x = (n); \
+ (p)->p_sizeh = __x >> 32; \
+ (p)->p_size = __x; \
} while (0)
#define DL_GETPOFFSET(p) (((u_int64_t)(p)->p_offseth << 32) +
(p)->p_offset)
#define DL_SETPOFFSET(p, n)do { \
- u_int64_t x = (n); \
- (p)->p_offseth = x >> 32; \
- (p)->p_offset = x; \
+ u_int64_t __x = (n); \
+ (p)->p_offseth = __x >> 32; \
+ (p)->p_offset = __x; \
} while (0)
#define DL_GETDSIZE(d)
Re: EV_SET(2) shadows variable
Philip Guenther wrote: > On Fri, 3 Apr 2020, Martin Pieuchot wrote: > > Thanks, here it is, ok? > > ok guenther@ Should we do the same to all other macros, just in case?
Re: EV_SET(2) shadows variable
On Fri, 3 Apr 2020, Martin Pieuchot wrote: > Thanks, here it is, ok? ok guenther@
Re: EV_SET(2) shadows variable
On 02/04/20(Thu) 13:36, Philip Guenther wrote:
> On Tue, Mar 31, 2020 at 11:24 PM Martin Pieuchot wrote:
>
> > The current form of EV_SET(2) declares a `kevp' variable. This can
> > cause to subtle bugs hard to discover if one uses a variable of the
> > same to retrieve events.
> >
> > Diff below prefixes the locally declared variable by an underscore,
> > like it it done in FD_ZERO(), which should be good enough to prevent
> > surprises.
> >
> > Is it the right way to correct such issue? How many underscores are
> > enough? Can the standards gurus tell me?
> >
>
> tl;dr: this should use a variable that starts with either two underbars
> (__kevp) or an underbar and a capital (_Kevp). The same is true of
> FD_ZERO().
> [...]
Thanks, here it is, ok?
Index: sys/event.h
===
RCS file: /cvs/src/sys/sys/event.h,v
retrieving revision 1.33
diff -u -p -r1.33 event.h
--- sys/event.h 20 Feb 2020 16:56:52 - 1.33
+++ sys/event.h 3 Apr 2020 07:47:17 -
@@ -42,14 +42,14 @@
#define EVFILT_SYSCOUNT8
-#define EV_SET(kevp_, a, b, c, d, e, f) do { \
- struct kevent *kevp = (kevp_); \
- (kevp)->ident = (a);\
- (kevp)->filter = (b); \
- (kevp)->flags = (c);\
- (kevp)->fflags = (d); \
- (kevp)->data = (e); \
- (kevp)->udata = (f);\
+#define EV_SET(kevp, a, b, c, d, e, f) do {\
+ struct kevent *__kevp = (kevp); \
+ (__kevp)->ident = (a); \
+ (__kevp)->filter = (b); \
+ (__kevp)->flags = (c); \
+ (__kevp)->fflags = (d); \
+ (__kevp)->data = (e); \
+ (__kevp)->udata = (f); \
} while(0)
struct kevent {
Index: sys/select.h
===
RCS file: /cvs/src/sys/sys/select.h,v
retrieving revision 1.17
diff -u -p -r1.17 select.h
--- sys/select.h12 Sep 2016 19:41:20 - 1.17
+++ sys/select.h3 Apr 2020 07:48:31 -
@@ -100,11 +100,11 @@ __fd_isset(int fd, const fd_set *p)
#defineFD_COPY(f, t) (void)(*(t) = *(f))
#endif
#defineFD_ZERO(p) do { \
- fd_set *_p = (p); \
- __size_t _n = __howmany(FD_SETSIZE, __NFDBITS); \
+ fd_set *__p = (p); \
+ __size_t __n = __howmany(FD_SETSIZE, __NFDBITS);\
\
- while (_n > 0) \
- _p->fds_bits[--_n] = 0; \
+ while (__n > 0) \
+ __p->fds_bits[--__n] = 0; \
} while (0)
#if __BSD_VISIBLE
Re: EV_SET(2) shadows variable
On Tue, Mar 31, 2020 at 11:24 PM Martin Pieuchot wrote:
> The current form of EV_SET(2) declares a `kevp' variable. This can
> cause to subtle bugs hard to discover if one uses a variable of the
> same to retrieve events.
>
> Diff below prefixes the locally declared variable by an underscore,
> like it it done in FD_ZERO(), which should be good enough to prevent
> surprises.
>
> Is it the right way to correct such issue? How many underscores are
> enough? Can the standards gurus tell me?
>
tl;dr: this should use a variable that starts with either two underbars
(__kevp) or an underbar and a capital (_Kevp). The same is true of
FD_ZERO().
The namespace of identifiers that begin with a single underbar is only
"reserved for use as identifiers with file scope in both the ordinary and
tag name spaces". That means it's perfectly legal for an application to
declare a *local* variable with such a name. So, this should be legal:
#include
#include
#include
#include
int main(void)
{
struct fd_set _p;
struct kevent _kevp;
FD_ZERO(&_p):
EV_SET(&_kevp, 0, 0, 0, 0, 0, 0);
return 0;
}
...but it currently blows up on the FD_ZERO() and would blow up in the
EV_SET() with your proposed diff.
Philip Guenther
EV_SET(2) shadows variable
The current form of EV_SET(2) declares a `kevp' variable. This can
cause to subtle bugs hard to discover if one uses a variable of the
same to retrieve events.
Diff below prefixes the locally declared variable by an underscore,
like it it done in FD_ZERO(), which should be good enough to prevent
surprises.
Is it the right way to correct such issue? How many underscores are
enough? Can the standards gurus tell me?
Index: sys/event.h
===
RCS file: /cvs/src/sys/sys/event.h,v
retrieving revision 1.33
diff -u -p -r1.33 event.h
--- sys/event.h 20 Feb 2020 16:56:52 - 1.33
+++ sys/event.h 1 Apr 2020 08:16:05 -
@@ -42,14 +42,14 @@
#define EVFILT_SYSCOUNT8
-#define EV_SET(kevp_, a, b, c, d, e, f) do { \
- struct kevent *kevp = (kevp_); \
- (kevp)->ident = (a);\
- (kevp)->filter = (b); \
- (kevp)->flags = (c);\
- (kevp)->fflags = (d); \
- (kevp)->data = (e); \
- (kevp)->udata = (f);\
+#define EV_SET(kevp, a, b, c, d, e, f) do {\
+ struct kevent *_kevp = (kevp); \
+ (_kevp)->ident = (a); \
+ (_kevp)->filter = (b); \
+ (_kevp)->flags = (c); \
+ (_kevp)->fflags = (d); \
+ (_kevp)->data = (e);\
+ (_kevp)->udata = (f); \
} while(0)
struct kevent {
