On Tue, 2023-08-22 at 10:16 +0000, Gerhard Roth wrote:
> On Tue, 2023-08-22 at 11:16 +0200, Martijn van Duren wrote:
> > On Mon, 2023-08-21 at 07:35 +0000, Gerhard Roth wrote:
> > > Hi Martijn,
> > >
> > > last November you fixed ber.c so that sequences won't generate
> > > an uninitialized subelement.
> > >
> > > This revealed another bug in ober_scanf_elements(): it couldn't
> > > process sequences with an empty list of subelements. The following
> > > code failed in ober_scanf_elements():
> > >
> > > struct ber_element *root;
> > > struct ber_element *sub;
> > >
> > > if ((root = ober_add_sequence(NULL)) == NULL)
> > > err(1, "ober_add_sequence() failed");
> > >
> > > errno = 0;
> > > if (ober_scanf_elements(root, "{e", &sub))
> > > err(1, "ober_scanf_elements() failed");
> > >
> > > printf("sub = %p\n", sub);
> > >
> > >
> > > The patch below fixes that.
> > >
> > > Gerhard
> > >
> > >
> > > Index: lib/libutil/ber.c
> > > ===================================================================
> > > RCS file: /cvs/src/lib/libutil/ber.c,v
> > > retrieving revision 1.24
> > > diff -u -p -u -p -r1.24 ber.c
> > > --- lib/libutil/ber.c 3 Nov 2022 17:58:10 -0000 1.24
> > > +++ lib/libutil/ber.c 21 Aug 2023 07:24:21 -0000
> > > @@ -700,7 +700,8 @@ ober_scanf_elements(struct ber_element *
> > >
> > > va_start(ap, fmt);
> > > while (*fmt) {
> > > - if (ber == NULL && *fmt != '$' && *fmt != '}' && *fmt !=
> > > ')')
> > > + if (ber == NULL && *fmt != '$' && *fmt != '}' && *fmt !=
> > > ')' &&
> > > + *fmt != 'e')
> > > goto fail;
> >
> > I'm not sure about this part. An ober_scanf_elements of "{}e" on your
> > example above also fails. The 'e' element might not increment the ber
> > pointer, but I do think it should fail if an expected element is
> > missing.
>
> You're right. And digging into it, only makes it look worse.
>
> I think, the 'e' format should behave differently.
>
> 1) in case of '{e' it is the first element of a sequence.
> Empty sequences are allowed and we should not fail but
> set the argument to NULL instead.
>
> 2) in all other cases, 'e' is the next element in the list.
> And here we should fail if there are less elements in the
> list than expected.
>
> Below is an updated diff that fixes the problem by remembering
> whether the last traversal was via 'be_sub' (fsub == 1) or 'be_next'
> and then behaves differently. Not very elegant although.
I'm not sure yet. Do you have a particular usecase for this?
Usually you would just do something like
ober_scanf_elements(elm, "e{", seq);
if (seq->be_sub != NULL) ...
>
> >
> > > switch (*fmt++) {
> > > case '$':
> > > @@ -797,7 +798,7 @@ ober_scanf_elements(struct ber_element *
> > > if (ber->be_encoding != BER_TYPE_SEQUENCE &&
> > > ber->be_encoding != BER_TYPE_SET)
> > > goto fail;
> > > - if (ber->be_sub == NULL || level >= _MAX_SEQ-1)
> > > + if (level >= _MAX_SEQ-1)
> >
> > This part is OK martijn@
> >
> > > goto fail;
> > > parent[++level] = ber;
> > > ber = ber->be_sub;
> > >
> >
>
> Index: lib/libutil/ber.c
> ===================================================================
> RCS file: /cvs/src/lib/libutil/ber.c,v
> retrieving revision 1.24
> diff -u -p -u -p -r1.24 ber.c
> --- lib/libutil/ber.c 3 Nov 2022 17:58:10 -0000 1.24
> +++ lib/libutil/ber.c 22 Aug 2023 10:10:43 -0000
> @@ -695,12 +695,14 @@ ober_scanf_elements(struct ber_element *
> off_t *pos;
> struct ber_oid *o;
> struct ber_element *parent[_MAX_SEQ], **e;
> + int fsub = 0;
>
> memset(parent, 0, sizeof(struct ber_element *) * _MAX_SEQ);
>
> va_start(ap, fmt);
> while (*fmt) {
> - if (ber == NULL && *fmt != '$' && *fmt != '}' && *fmt != ')')
> + if (ber == NULL && *fmt != '$' && *fmt != '}' && *fmt != ')' &&
> + *fmt != 'e')
> goto fail;
> switch (*fmt++) {
> case '$':
> @@ -732,6 +734,8 @@ ober_scanf_elements(struct ber_element *
> case 'e':
> e = va_arg(ap, struct ber_element **);
> *e = ber;
> + if (fsub)
> + goto fail;
> ret++;
> continue;
> case 'E':
> @@ -797,10 +801,11 @@ ober_scanf_elements(struct ber_element *
> if (ber->be_encoding != BER_TYPE_SEQUENCE &&
> ber->be_encoding != BER_TYPE_SET)
> goto fail;
> - if (ber->be_sub == NULL || level >= _MAX_SEQ-1)
> + if (level >= _MAX_SEQ-1)
> goto fail;
> parent[++level] = ber;
> ber = ber->be_sub;
> + fsub = 1;
> ret++;
> continue;
> case '}':
> @@ -808,6 +813,7 @@ ober_scanf_elements(struct ber_element *
> if (level < 0 || parent[level] == NULL)
> goto fail;
> ber = parent[level--];
> + fsub = 0;
> ret++;
> break;
> default:
>
>
