On Tue, 2023-08-22 at 13:27 +0200, Martijn van Duren wrote:
> 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) ...
In my use-case the format contained more elements that followed.
I wanted to scan them all with a single ober_scanf_elements().
But I can just as well split that up into two separate call.
So let's forget about this patch.
Many thanks for looking into this!
Gerhard
>
> >
> > >
> > > > 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:
> >
> >
>
smime.p7s
Description: S/MIME cryptographic signature
