ber_oid_cmp don't rely on 0 elements

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

ber_oid_cmp don't rely on 0 elements

Martijn van Duren-5
Similar edgecase to ber_get_oid, but doesn't seem to influence snmpctl  
walk. It does however influence another project of mine.

snmpd returns ip addresses for e.g. inetCidrRouteIfIndex, which can
contain 0 elements in the mibs. This can result in false equals for
ber_oid_cmp. Diff below rebases the end of mib check on the bo_n value.

Fixes the following example (based on snmpd returned consecutive oids):
$ cat ./test.c
#include <stdio.h>
#include <ber.h>

int
main(int argc, char *argv[])
{
        struct ber_oid oid1, oid2;

        ber_string2oid(
            "1.3.6.1.2.1.4.24.7.1.7.1.4.127.0.0.0.8.2.0.8.1.4.127.0.0.1",
             &oid1);
        ber_string2oid(
            "1.3.6.1.2.1.4.24.7.1.7.1.4.127.0.0.1.32.2.0.1.1.4.127.0.0.1",
            &oid2);

        printf("%d\n", ber_oid_cmp(&oid1, &oid2));
}
$ LDFLAGS=-lutil make test && ./test
0
$ LDFLAGS=-lutil make test && LD_PRELOAD=/usr/src/lib/libutil/obj/libutil.so.13.1  ./test
`test' is up to date.
1

OK?

martijn@

Index: ber.c
===================================================================
RCS file: /cvs/src/lib/libutil/ber.c,v
retrieving revision 1.9
diff -u -p -r1.9 ber.c
--- ber.c 1 Jun 2019 19:40:05 -0000 1.9
+++ ber.c 31 Jul 2019 20:21:51 -0000
@@ -456,23 +456,23 @@ int
 ber_oid_cmp(struct ber_oid *a, struct ber_oid *b)
 {
  size_t i;
- for (i = 0; i < BER_MAX_OID_LEN; i++) {
- if (a->bo_id[i] != 0) {
- if (a->bo_id[i] == b->bo_id[i])
- continue;
- else if (a->bo_id[i] < b->bo_id[i]) {
- /* b is a successor of a */
- return (1);
- } else {
- /* b is a predecessor of a */
- return (-1);
- }
- } else if (b->bo_id[i] != 0) {
- /* b is larger, but a child of a */
- return (2);
- } else
- break;
+ for (i = 0; i < a->bo_n && i < b->bo_n; i++) {
+ if (a->bo_id[i] == b->bo_id[i])
+ continue;
+ else if (a->bo_id[i] < b->bo_id[i]) {
+ /* b is a successor of a */
+ return (1);
+ } else {
+ /* b is a predecessor of a */
+ return (-1);
+ }
  }
+ /* b is larger, but a child of a */
+ if (a->bo_n < b->bo_n)
+ return (2);
+ /* b is a predecessor of a */
+ if (a->bo_n > b->bo_n)
+ return -1;
 
  /* b and a are identical */
  return (0);

Reply | Threaded
Open this post in threaded view
|

Re: ber_oid_cmp don't rely on 0 elements

Claudio Jeker
On Wed, Jul 31, 2019 at 10:27:58PM +0200, Martijn van Duren wrote:

> Similar edgecase to ber_get_oid, but doesn't seem to influence snmpctl  
> walk. It does however influence another project of mine.
>
> snmpd returns ip addresses for e.g. inetCidrRouteIfIndex, which can
> contain 0 elements in the mibs. This can result in false equals for
> ber_oid_cmp. Diff below rebases the end of mib check on the bo_n value.
>
> Fixes the following example (based on snmpd returned consecutive oids):
> $ cat ./test.c
> #include <stdio.h>
> #include <ber.h>
>
> int
> main(int argc, char *argv[])
> {
> struct ber_oid oid1, oid2;
>
> ber_string2oid(
>    "1.3.6.1.2.1.4.24.7.1.7.1.4.127.0.0.0.8.2.0.8.1.4.127.0.0.1",
>     &oid1);
> ber_string2oid(
>    "1.3.6.1.2.1.4.24.7.1.7.1.4.127.0.0.1.32.2.0.1.1.4.127.0.0.1",
>    &oid2);
>
> printf("%d\n", ber_oid_cmp(&oid1, &oid2));
> }
> $ LDFLAGS=-lutil make test && ./test
> 0
> $ LDFLAGS=-lutil make test && LD_PRELOAD=/usr/src/lib/libutil/obj/libutil.so.13.1  ./test
> `test' is up to date.
> 1
>
> OK?

Absolutly. What bothers me a bit is that ber_oid_cmp() is not symetrical
 ber_oid_cmp(a, b) == -ber_oid_cmp(b, a)
but it is documented like that.

OK claudio@
 

> martijn@
>
> Index: ber.c
> ===================================================================
> RCS file: /cvs/src/lib/libutil/ber.c,v
> retrieving revision 1.9
> diff -u -p -r1.9 ber.c
> --- ber.c 1 Jun 2019 19:40:05 -0000 1.9
> +++ ber.c 31 Jul 2019 20:21:51 -0000
> @@ -456,23 +456,23 @@ int
>  ber_oid_cmp(struct ber_oid *a, struct ber_oid *b)
>  {
>   size_t i;
> - for (i = 0; i < BER_MAX_OID_LEN; i++) {
> - if (a->bo_id[i] != 0) {
> - if (a->bo_id[i] == b->bo_id[i])
> - continue;
> - else if (a->bo_id[i] < b->bo_id[i]) {
> - /* b is a successor of a */
> - return (1);
> - } else {
> - /* b is a predecessor of a */
> - return (-1);
> - }
> - } else if (b->bo_id[i] != 0) {
> - /* b is larger, but a child of a */
> - return (2);
> - } else
> - break;
> + for (i = 0; i < a->bo_n && i < b->bo_n; i++) {
> + if (a->bo_id[i] == b->bo_id[i])
> + continue;
> + else if (a->bo_id[i] < b->bo_id[i]) {
> + /* b is a successor of a */
> + return (1);
> + } else {
> + /* b is a predecessor of a */
> + return (-1);
> + }
>   }
> + /* b is larger, but a child of a */
> + if (a->bo_n < b->bo_n)
> + return (2);
> + /* b is a predecessor of a */
> + if (a->bo_n > b->bo_n)
> + return -1;
>  
>   /* b and a are identical */
>   return (0);
>

--
:wq Claudio