On Tue, Jul 03, 2018 at 03:45:49PM -0400, Rob Pierce wrote:
> On Sat, Jun 30, 2018 at 02:04:16PM -0400, Rob Pierce wrote:
> > I recently committed a piece of BER code synchronizing in the wrong
> > direction
> > (i.e. from the ldap instances to the snmpd instance). sthen@ noticed a break
> > in SNMPv3 authentication and reverted that part of the change. Thanks
> > Stuart!
> >
> > I just fixed some typos in the snmpd regression test which prevented me from
> > noticing the problem. I am also the author of those typos... arg.
> >
> > I believe the diff below synchronizes this piece of BER code in the right
> > direction, from the snmpd instance to the ldap instances. I have done some
> > testing against ldap and ldapd, but not ypldap. If anyone out there could
> > perform further testing on and/or review the change to make sure it doesn't
> > break anything that would be much appreciated.
> >
> > Thanks!
> >
> > Rob
> >
> > Index: usr.bin/ldap/ber.c
> > ===================================================================
> > RCS file: /cvs/src/usr.bin/ldap/ber.c,v
> > retrieving revision 1.6
> > diff -u -p -r1.6 ber.c
> > --- usr.bin/ldap/ber.c 29 Jun 2018 18:28:41 -0000 1.6
> > +++ usr.bin/ldap/ber.c 30 Jun 2018 17:50:06 -0000
> > @@ -1240,7 +1240,7 @@ ber_free(struct ber *b)
> > static ssize_t
> > ber_getc(struct ber *b, u_char *c)
> > {
> > - return ber_readbuf(b, c, 1);
> > + return ber_read(b, c, 1);
> > }
> >
> > static ssize_t
> >
> > Index: usr.sbin/ldapd/ber.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/ldapd/ber.c,v
> > retrieving revision 1.16
> > diff -u -p -r1.16 ber.c
> > --- usr.sbin/ldapd/ber.c 29 Jun 2018 18:28:42 -0000 1.16
> > +++ usr.sbin/ldapd/ber.c 30 Jun 2018 17:50:06 -0000
> > @@ -1240,7 +1240,7 @@ ber_free(struct ber *b)
> > static ssize_t
> > ber_getc(struct ber *b, u_char *c)
> > {
> > - return ber_readbuf(b, c, 1);
> > + return ber_read(b, c, 1);
> > }
> >
> > static ssize_t
> >
> > Index: usr.sbin/ypldap/ber.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/ypldap/ber.c,v
> > retrieving revision 1.18
> > diff -u -p -r1.18 ber.c
> > --- usr.sbin/ypldap/ber.c 29 Jun 2018 18:28:42 -0000 1.18
> > +++ usr.sbin/ypldap/ber.c 30 Jun 2018 17:50:06 -0000
> > @@ -1240,7 +1240,7 @@ ber_free(struct ber *b)
> > static ssize_t
> > ber_getc(struct ber *b, u_char *c)
> > {
> > - return ber_readbuf(b, c, 1);
> > + return ber_read(b, c, 1);
> > }
> >
> > static ssize_t
>
> After further review and testing I am now looking for ok's on this.
>
> Ok?
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?
--
: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
