On Wed, Nov 02, 2022 at 05:56:21PM +0100, Martijn van Duren wrote:
> On Wed, 2022-11-02 at 17:47 +0100, Claudio Jeker wrote:
> > 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.
>
> Ah ok, so confirming that the change is OK. Thanks.
> >
> > > >
> > > > > @@ -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?
>
> Yes, aldap.c and snmpe.c do this. And even if it didn't, going with
> EINVAL is more concise than ECANCELED.
I really dislike such errno magic but fixing that is a much bigger
project.
> So is the comment OK as is, do you still want to go with your comment,
> or do you want to suggest something else?
My question is should we actually write this as:
if (r == -1 || r > len) {
errno = EINVAL;
return -1;
}
In the end any error of a subelement can be interpreted as an illegal seq/set.
> > > >
> > > > /* 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.
>
> Yes, but the question still remains: Do I need to add a comment here?
No
> >
> > > >
> > > > > + 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
