On Thu, 28 Jun 2018 09:54:34 +0200
Claudio Jeker <[email protected]> wrote:
> On Wed, Jun 27, 2018 at 06:42:58PM -0400, Rob Pierce wrote:
> > The following diff makes ber.c and ber.h identical across ldap,
> > ldapd, and ypldap, and slightly reduces the diff with snmpd.
> >
> > It covers the evolution of a few scattered enhancements, including:
> >
> > - sync proscription of indefinite length BER encoding
> > - sync consistent presence of ber_free_element()
> > - sync correct parsing of '}' and ')' in ber_scanf_elements()
> > (i.e. a "break" instead of a "continue")
> > - sync use of ber_readbuf in ber_getc()
> >
> > Ok?
>
> OK claudio@
>
> One note inline
>
> > Index: usr.bin/ldap/ber.c
> > ===================================================================
> > RCS file: /cvs/src/usr.bin/ldap/ber.c,v
> > retrieving revision 1.4
> > diff -u -p -r1.4 ber.c
> > --- usr.bin/ldap/ber.c 27 Jun 2018 20:38:10 -0000 1.4
> > +++ usr.bin/ldap/ber.c 27 Jun 2018 22:15:55 -0000
> > @@ -729,7 +729,7 @@ ber_scanf_elements(struct ber_element *b
> > goto fail;
> > ber = parent[level--];
> > ret++;
> > - continue;
> > + break;
> > default:
> > goto fail;
> > }
> > @@ -822,6 +822,19 @@ ber_read_elements(struct ber *ber, struc
> > }
> >
> > void
> > +ber_free_element(struct ber_element *root)
> > +{
> > + if (root->be_sub && (root->be_encoding ==
> > BER_TYPE_SEQUENCE ||
> > + root->be_encoding == BER_TYPE_SET))
> > + ber_free_elements(root->be_sub);
> > + if (root->be_free && (root->be_encoding ==
> > BER_TYPE_OCTETSTRING ||
> > + root->be_encoding == BER_TYPE_BITSTRING ||
> > + root->be_encoding == BER_TYPE_OBJECT))
> > + free(root->be_val);
>
> I find this a bit strange. Why is be_free only respected for the
> specified encodings? I know ber_free_elements() does the same.
> Also ber_free_element() and ber_free_elements() could share more code.
> e.g.
>
> void
> ber_free_element(struct ber_element *root)
> {
> root->be_next = NULL;
> ber_free_elements(root);
> }
>
> Or the other way around...
>
> Was also wondering if ber_free_element() should return root->be_next
> since that could make the calling code a bit nicer.
>
> Anyway this is more food for thoughts and not related to this patch.
As to be_free, only those three encodings BER_TYPE_OCTETSTRING,
BER_TYPE_BITSTRING, and BER_TYPE_OBJECT have malloc()'d their memory.
ber_free_element() is my fault; I introduced it for LDAP attribute value
deletion (in ldapd, attributes.c:ldap_del_values(). I simply copied
the code from ber_free_elements() and removed the “offending” line.
Your code sharing example is the better way.
Not sure about ber_free_element returning root->be_next, though the
calling could would indeed look nice:
ber_link_elements(prev,
ber_free_element(ber_unlink_elements(prev));
Best regards,
Robert
>
> > + free(root);
[rest deleted]
