On Wed, Nov 02, 2022 at 05:25:12PM +0100, Martijn van Duren wrote:
> On Wed, 2022-11-02 at 17:00 +0100, Claudio Jeker wrote:
> > On Wed, Nov 02, 2022 at 07:33:14AM +0100, Martijn van Duren wrote:
> > > I found 2 minor issues in the handling of sequences/sets in
> > > ober_read_element():
> > > 1) An empty sequence/set (which is basically always) unconditionally
> > > creates an (uninitialised) sub-element. Add the same length check
> > > used to check the next element
> > > 2) For each sub-element r is only checked for -1, but not if it
> > > overflows the length of the sequence itself. This is not a big risk
> > > since each sub-element is length-checked against the buffer
> > > availability and simply returns an ECANCELED, which would be no
> > > worse memory-wise than sending an extremely large packet.
> > >
> > > While here, only having the NULL of the comparison on the next line
> > > annoyed me.
> > >
> > > OK?
> >
> > See below.
> >
> > > martijn@
> > >
> > > Index: ber.c
> > > ===================================================================
> > > RCS file: /cvs/src/lib/libutil/ber.c,v
> > > retrieving revision 1.23
> > > diff -u -p -r1.23 ber.c
> > > --- ber.c 21 Oct 2021 08:17:33 -0000 1.23
> > > +++ ber.c 2 Nov 2022 06:28:33 -0000
> > > @@ -1375,7 +1375,7 @@ ober_read_element(struct ber *ber, struc
> > > break;
> > > case BER_TYPE_SEQUENCE:
> > > case BER_TYPE_SET:
> > > - if (elm->be_sub == NULL) {
> > > + if (len > 0 && elm->be_sub == NULL) {
> > > if ((elm->be_sub = ober_get_element(0)) == NULL)
> > > return -1;
> > > }
> >
> > OK, be_sub == NULL is something checked and better than an empty object.
>
> I'm not sure I understand what you mean here.
I just wanted to say that elm->be_sub == NULL is a condition that most
other code handles well. While having an empty element in elm->be_sub is
something other code may not handle well.
> >
> > > @@ -1390,13 +1390,21 @@ ober_read_element(struct ber *ber, struc
> > > return -1;
> > > }
> > > r = ober_read_element(ber, next);
> > > - if (r == -1)
> > > + if (r == -1) {
> > > + /* sub-element overflows sequence/set */
> >
> > This comment is not quite right. I think the proper comment here is:
>
> I'm not sure. Yes the sub-element overflows the buffer, so it is more
> accurate in the literal sense. However, since the sequence/set already
> has its entire length in the buffer it implies that an ECANCELED is an
> overflow of the sub-element of the sequence and therefore makes it
> clearer why the errno is overwritten.
Actually why do we need to reset the errno anyway?
Is anything in userland looking at ECANCELED and retries after receiving
more data?
> >
> > /* sub-element overflows buffer */
> > > + if (errno == ECANCELED)
> > > + errno = EINVAL;
> > > return -1;
> > > + }
> > > + if (r > len) {
> >
> > This check here is actually checking that the sub_element does not
> > overflow the sequence/set. So maybe move the abcve comment down here.
>
> I think it does exactly the same thing, with the only difference that
> the ECANCELED case didn't reside fully in the buffer. The reason I
> didn't add the comment here was because I think it's clear from the
> context, where that context might be less obvious at ECANCELED.
Sure. It is kind of same same. The check is important though.
> >
> > > + errno = EINVAL;
> > > + return -1;
> > > + }
> > > elements++;
> > > len -= r;
> > > if (len > 0 && next->be_next == NULL) {
> > > - if ((next->be_next = ober_get_element(0)) ==
> > > - NULL)
> > > + next->be_next = ober_get_element(0);
> > > + if (next->be_next == NULL)
> > > return -1;
> > > }
> > > next = next->be_next;
> > >
> >
> > Apart from that OK claudio@
> >
>
--
:wq Claudio
