Re: ber.c: Fix some minor issues in ober_read_element()
On Wed, 2022-11-02 at 18:34 +0100, Claudio Jeker wrote: > 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 - 1.23 > > > > > > +++ ber.c 2 Nov 2022 06:28:33 - > > > > > > @@ -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. There are a few ERANGE cases in the ber_read_elements code, which are valid ber, but too big for our ber.c code. So I don't think that statement is definitionally true. I agree that the errno magic isn't pretty, but I don't see a clean way to implement the fix without the errno magic. So unless we want a too wide brush I guess my original diff still stands? > > > > > > > > > > > /* sub-element overflows buffer */ > > > > > > + if (errno == ECANCELED) > > > > > > + errno = EINVAL; > > > > > > return -1; > > > > > > + } > > > > > > + if (r > len) { > > > > > > > >
Re: ber.c: Fix some minor issues in ober_read_element()
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 - 1.23 > > > > > +++ ber.c 2 Nov 2022 06:28:33 - > > > > > @@ -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
Re: ber.c: Fix some minor issues in ober_read_element()
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 - 1.23 > > > > +++ ber.c 2 Nov 2022 06:28:33 - > > > > @@ -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. So is the comment OK as is, do you still want to go with your comment, or do you want to suggest something else? > > > > > > > /* 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? > > > > > > > > + 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) > > > >
Re: ber.c: Fix some minor issues in ober_read_element()
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 - 1.23 > > > +++ ber.c 2 Nov 2022 06:28:33 - > > > @@ -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
Re: ber.c: Fix some minor issues in ober_read_element()
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 - 1.23 > > +++ ber.c 2 Nov 2022 06:28:33 - > > @@ -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. > > > @@ -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. > > /* 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. > > > + 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@ >
Re: ber.c: Fix some minor issues in ober_read_element()
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 - 1.23 > +++ ber.c 2 Nov 2022 06:28:33 - > @@ -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. > @@ -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: /* 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. > + 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
ber.c: Fix some minor issues in ober_read_element()
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? 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 - 1.23 +++ ber.c 2 Nov 2022 06:28:33 - @@ -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; } @@ -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 */ + if (errno == ECANCELED) + errno = EINVAL; return -1; + } + if (r > len) { + 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;