On Tue, Jul 03, 2018 at 05:38:18PM -0400, Rob Pierce wrote:
> On Tue, Jul 03, 2018 at 09:25:06PM +0100, Stuart Henderson wrote:
> > On 2018/07/03 22:17, Claudio Jeker wrote:
> > > I have a hard time to understand why this is needed in snmpd.
> > > For single char reads ber_readbuf(b, c, 1) and ber_read(b, c, 1) should do
> > > exaclty the same. At least in the old code. I see that snmpd added br_offs
> > > in a way that causes this breakage. The update of br_offs is handled in
> > > the wrong place in my opinion.
> > >
> > > I would prefer something like the appended diff.
> > > This also removes the r == 0 check in ber_read since ber_readbuf cannot
> > > return 0. Can someone check against the snmpd usecase that failed?
> >
> > Yes this fixes it, thanks. The use case that fails is "configure SNMPv3,
> > try to use it".
> > e.g.
> >
> > user "username" authkey "authkey" enc aes enckey "privkey"
> >
> > $ snmpwalk -v3 -l authPriv -a SHA -A authkey -u username -x AES -X privkey
> > hostname
> >
> > > --
> > > :wq Claudio
> > >
> > > Index: ber.c
> > > ===================================================================
> > > RCS file: /cvs/src/usr.sbin/snmpd/ber.c,v
> > > retrieving revision 1.37
> > > diff -u -p -r1.37 ber.c
> > > --- ber.c 1 Jul 2018 20:03:48 -0000 1.37
> > > +++ ber.c 3 Jul 2018 20:16:13 -0000
> > > @@ -1212,8 +1212,7 @@ ber_read_element(struct ber *ber, struct
> > > static ssize_t
> > > ber_readbuf(struct ber *b, void *buf, size_t nbytes)
> > > {
> > > - size_t sz;
> > > - size_t len;
> > > + size_t sz, len;
> > >
> > > if (b->br_rbuf == NULL)
> > > return -1;
> > > @@ -1227,6 +1226,7 @@ ber_readbuf(struct ber *b, void *buf, si
> > >
> > > bcopy(b->br_rptr, buf, len);
> > > b->br_rptr += len;
> > > + b->br_offs += len;
> > >
> > > return (len);
> > > }
> > > @@ -1271,7 +1271,7 @@ ber_free(struct ber *b)
> > > static ssize_t
> > > ber_getc(struct ber *b, u_char *c)
> > > {
> > > - return ber_read(b, c, 1);
> > > + return ber_readbuf(b, c, 1);
> > > }
> > >
> > > static ssize_t
> > > @@ -1284,14 +1284,10 @@ ber_read(struct ber *ber, void *buf, siz
> > > r = ber_readbuf(ber, b, remain);
> > > if (r == -1)
> > > return -1;
> > > - if (r == 0)
> > > - return (b - (u_char *)buf);
> > > b += r;
> > > remain -= r;
> > > }
> > > - r = b - (u_char *)buf;
> > > - ber->br_offs += r;
> > > - return r;
> > > + return (b - (u_char *)buf);
> > > }
> > >
> > > int
> > >
>
> Your diff also passes the snmpd regression tests here, including that specific
> use case. LDAP also appears happy with this change.
>
> How about this larger diff (based on yours) for all ber.c instances?
>
> Index: usr.bin/ldap/ber.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/ldap/ber.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 ber.c
> --- usr.bin/ldap/ber.c 3 Jul 2018 18:49:10 -0000 1.8
> +++ usr.bin/ldap/ber.c 3 Jul 2018 21:33:26 -0000
> @@ -1212,8 +1212,7 @@ ber_read_element(struct ber *ber, struct
> static ssize_t
> ber_readbuf(struct ber *b, void *buf, size_t nbytes)
> {
> - size_t sz;
> - size_t len;
> + size_t sz, len;
>
> if (b->br_rbuf == NULL)
> return -1;
> @@ -1227,6 +1226,7 @@ ber_readbuf(struct ber *b, void *buf, si
>
> bcopy(b->br_rptr, buf, len);
> b->br_rptr += len;
> + b->br_offs += len;
>
> return (len);
> }
> @@ -1284,14 +1284,10 @@ ber_read(struct ber *ber, void *buf, siz
> r = ber_readbuf(ber, b, remain);
> if (r == -1)
> return -1;
> - if (r == 0)
> - return (b - (u_char *)buf);
> b += r;
> remain -= r;
> }
> - r = b - (u_char *)buf;
> - ber->br_offs += r;
> - return r;
> + return (b - (u_char *)buf);
> }
>
> int
> Index: usr.sbin/ldapd/ber.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ldapd/ber.c,v
> retrieving revision 1.18
> diff -u -p -r1.18 ber.c
> --- usr.sbin/ldapd/ber.c 3 Jul 2018 18:49:10 -0000 1.18
> +++ usr.sbin/ldapd/ber.c 3 Jul 2018 21:33:31 -0000
> @@ -1212,8 +1212,7 @@ ber_read_element(struct ber *ber, struct
> static ssize_t
> ber_readbuf(struct ber *b, void *buf, size_t nbytes)
> {
> - size_t sz;
> - size_t len;
> + size_t sz, len;
>
> if (b->br_rbuf == NULL)
> return -1;
> @@ -1227,6 +1226,7 @@ ber_readbuf(struct ber *b, void *buf, si
>
> bcopy(b->br_rptr, buf, len);
> b->br_rptr += len;
> + b->br_offs += len;
>
> return (len);
> }
> @@ -1284,14 +1284,10 @@ ber_read(struct ber *ber, void *buf, siz
> r = ber_readbuf(ber, b, remain);
> if (r == -1)
> return -1;
> - if (r == 0)
> - return (b - (u_char *)buf);
> b += r;
> remain -= r;
> }
> - r = b - (u_char *)buf;
> - ber->br_offs += r;
> - return r;
> + return (b - (u_char *)buf);
> }
>
> int
> Index: usr.sbin/ypldap/ber.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ypldap/ber.c,v
> retrieving revision 1.20
> diff -u -p -r1.20 ber.c
> --- usr.sbin/ypldap/ber.c 3 Jul 2018 18:49:10 -0000 1.20
> +++ usr.sbin/ypldap/ber.c 3 Jul 2018 21:33:38 -0000
> @@ -1212,8 +1212,7 @@ ber_read_element(struct ber *ber, struct
> static ssize_t
> ber_readbuf(struct ber *b, void *buf, size_t nbytes)
> {
> - size_t sz;
> - size_t len;
> + size_t sz, len;
>
> if (b->br_rbuf == NULL)
> return -1;
> @@ -1227,6 +1226,7 @@ ber_readbuf(struct ber *b, void *buf, si
>
> bcopy(b->br_rptr, buf, len);
> b->br_rptr += len;
> + b->br_offs += len;
>
> return (len);
> }
> @@ -1284,14 +1284,10 @@ ber_read(struct ber *ber, void *buf, siz
> r = ber_readbuf(ber, b, remain);
> if (r == -1)
> return -1;
> - if (r == 0)
> - return (b - (u_char *)buf);
> b += r;
> remain -= r;
> }
> - r = b - (u_char *)buf;
> - ber->br_offs += r;
> - return r;
> + return (b - (u_char *)buf);
> }
>
> int
> Index: usr.sbin/snmpd/ber.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/snmpd/ber.c,v
> retrieving revision 1.37
> diff -u -p -r1.37 ber.c
> --- usr.sbin/snmpd/ber.c 1 Jul 2018 20:03:48 -0000 1.37
> +++ usr.sbin/snmpd/ber.c 3 Jul 2018 21:33:39 -0000
> @@ -1212,8 +1212,7 @@ ber_read_element(struct ber *ber, struct
> static ssize_t
> ber_readbuf(struct ber *b, void *buf, size_t nbytes)
> {
> - size_t sz;
> - size_t len;
> + size_t sz, len;
>
> if (b->br_rbuf == NULL)
> return -1;
> @@ -1227,6 +1226,7 @@ ber_readbuf(struct ber *b, void *buf, si
>
> bcopy(b->br_rptr, buf, len);
> b->br_rptr += len;
> + b->br_offs += len;
>
> return (len);
> }
> @@ -1271,7 +1271,7 @@ ber_free(struct ber *b)
> static ssize_t
> ber_getc(struct ber *b, u_char *c)
> {
> - return ber_read(b, c, 1);
> + return ber_readbuf(b, c, 1);
> }
>
> static ssize_t
> @@ -1284,14 +1284,10 @@ ber_read(struct ber *ber, void *buf, siz
> r = ber_readbuf(ber, b, remain);
> if (r == -1)
> return -1;
> - if (r == 0)
> - return (b - (u_char *)buf);
> b += r;
> remain -= r;
> }
> - r = b - (u_char *)buf;
> - ber->br_offs += r;
> - return r;
> + return (b - (u_char *)buf);
> }
>
> int
>
Looks good to me. I made the same for some of them already.
--
:wq Claudio
