[Bug 1001789] New: Null pointer in snmp_send_trap and byte order inconsistency

[Bug 1001789] New: Null pointer in snmp_send_trap and byte order inconsistency

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001789

            Bug ID: 1001789
           Summary: Null pointer in snmp_send_trap and byte order
                    inconsistency
           Product: eCos
           Version: CVS
            Target: All
  Architecture/Host All
                OS:
            Status: UNCONFIRMED
          Severity: normal
          Priority: low
         Component: lwIP
          Assignee: unassigned@bugs.ecos.sourceware.org
          Reporter: mjones@linear.com
                CC: ecos-bugs@ecos.sourceware.org

I have found a couple of problems using lwip/snmp. I will describe them here
and let let someone that knows lwip deal with them because I don't know the
intent of the design and I would have a hard time deciding how to improve it.

The first problem is snap_send_trap has a call to ip_route(...) shown below. If
the IP address of the target platform does not match the trap destination, it
returns a null. For example, with netmask 255.255.255.0, if the target platform
is 192.168.0.2 and the trap destination is 192.168.1.5, ip_route(...) will
return null. Then the code will operate on a null pointer.

I think the call should either return an error, set an err value, or use an
ASSERT, etc. Most people probably don't have a problem because they are on the
same subnet. I only ran into the problem because I did not know that the
ip_route() could not handle the more complex routing.

snmp_send_trap(s8_t generic_trap, struct snmp_obj_id *eoid, s32_t
specific_trap)
{
  struct snmp_trap_dst *td;
  struct netif *dst_if;
  struct ip_addr dst_ip;
  struct pbuf *p;
  u16_t i,tot_len;

  for (i=0, td = &trap_dst[0]; i<SNMP_TRAP_DESTINATIONS; i++, td++)
  {
    if ((td->enable != 0) && (td->dip.addr != 0))
    {
      /* network order trap destination */
      trap_msg.dip.addr = td->dip.addr;
      /* lookup current source address for this dst */
      dst_if = ip_route(&td->dip);
      dst_ip.addr = ntohl(dst_if->ip_addr.addr);

The next problem is an inconsistency in IP representation handling. This code
below works because it calls htonl(...) before snap_trap_dst_ip_set(...), which
has another htonl(...) call. Most of the lwip takes an address in the form
returned by IP4_ADDR(...). This inconsistency lead to some debug time trying to
figure out why it did not work.

struct ip_addr addr;
IP4_ADDR(&addr, 192, 168, 1, 102);
addr.addr = htonl(snmp_message->ADDRESS.addr);
snmp_trap_dst_ip_set(0,&addr);

Because this was tricky to make work the first time, I leave my code here for
the next person trying this. I have not tested if this leaks memory yet, so if
you use it, beware it might leak. The context is this code receives a message
from a mailbox in an lwip thread. I don't know if I really need the callback or
not. I found an example working in a similar way and used it. If nothing else
this gives the context of how I am using lwip when I have the above two
problems.

// Code in lwip thread

struct ip_addr addr;
addr.addr = htonl(snmp_message->ADDRESS.addr);
snmp_trap_dst_ip_set(0,&addr);
snmp_trap_dst_enable(0,snmp_message->TRAP);

struct snmp_varbind *vb = snmp_varbind_alloc(&objid, SNMP_ASN1_OPAQUE,
strlen(snmp_message->message));
struct snmp_varbind_root *vb_root = (struct snmp_varbind_root
*)malloc(sizeof(struct snmp_varbind_root));;
memset(vb_root, 0, sizeof(struct snmp_varbind_root));
memcpy (vb->value, snmp_message->message, strlen(snmp_message->message));
snmp_varbind_tail_add(vb_root,vb);
tcpip_callback(send_trap_callback, vb_root);

void send_trap_callback( void * parameters )
{

    struct snmp_varbind_root *vb_root;

    if( parameters != NULL )
    {
        vb_root = (struct snmp_varbind_root *) parameters;
        struct snmp_obj_id eoid;
        // there should be a way to do this directly.
        eoid.id[0] = enterprises_ids[1];
        eoid.len = 1;

        trap_msg.outvb = *vb_root;

        snmp_send_trap(SNMP_GENTRAP_ENTERPRISESPC, &eoid, 1);

        // Need to know what should be freed.
        // message text?
        // struct snmp_varbind?
        // struct snmp_varbind_root?
    }
}

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 1001789] Null pointer in snmp_send_trap and byte order inconsistency

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001789

John Dallaway <john@dallaway.org.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEEDINFO
                 CC|                            |john@dallaway.org.uk
     Ever confirmed|0                           |1

--- Comment #1 from John Dallaway <john@dallaway.org.uk> ---
(In reply to comment #0)

> The next problem is an inconsistency in IP representation handling. This
> code below works because it calls htonl(...) before
> snap_trap_dst_ip_set(...), which has another htonl(...) call. Most of the
> lwip takes an address in the form returned by IP4_ADDR(...). This
> inconsistency lead to some debug time trying to figure out why it did not
> work.

Ref: http://savannah.nongnu.org/bugs/?29256

It looks like this issue was fixed upstream as part of the following commit:

http://git.savannah.gnu.org/cgit/lwip.git/commit/?id=d0f1c552e2146825d011f106672698244eea01dc

Mike, can you prepare an equivalent fix for the eCos lwIP sources?

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 1001789] Null pointer in snmp_send_trap and byte order inconsistency

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001789

--- Comment #2 from Mike Jones <mjones@linear.com> ---
Created attachment 2115
  --> http://bugs.ecos.sourceware.org/attachment.cgi?id=2115&action=edit
htonl patch

This fixes the problem with the byte reversal by applying lwip bugfix #29256.
The eCos older version does not have one call (ip_addr_set_hton), so the
modification is slightly different. I retested my app. Note that after applying
this patch, application code with a working around using htonl(..) will break
and the htonl(..) call will have to be removed.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 1001789] Null pointer in snmp_send_trap and byte order inconsistency

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001789

Mike Jones <mjones@linear.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEEDINFO                    |MODIFIED

--- Comment #3 from Mike Jones <mjones@linear.com> ---
Is there anything keeping this from being committed? This is a patch from the
LwIP bugfix database.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 1001789] Null pointer in snmp_send_trap and byte order inconsistency

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001789

John Dallaway <john@dallaway.org.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|MODIFIED                    |RESOLVED
         Resolution|---                         |CURRENTRELEASE
           Assignee|unassigned@bugs.ecos.source |john@dallaway.org.uk
                   |ware.org                    |

--- Comment #4 from John Dallaway <john@dallaway.org.uk> ---
Upstream fix applied. Mike, thank you for investigating and preparing the patch
file.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 1001789] New: Null pointer in snmp_send_trap and byte order inconsistency

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001789

            Bug ID: 1001789
           Summary: Null pointer in snmp_send_trap and byte order
                    inconsistency
           Product: eCos
           Version: CVS
            Target: All
  Architecture/Host All
                OS:
            Status: UNCONFIRMED
          Severity: normal
          Priority: low
         Component: lwIP
          Assignee: unassigned@bugs.ecos.sourceware.org
          Reporter: mjones@linear.com
                CC: ecos-bugs@ecos.sourceware.org

I have found a couple of problems using lwip/snmp. I will describe them here
and let let someone that knows lwip deal with them because I don't know the
intent of the design and I would have a hard time deciding how to improve it.

The first problem is snap_send_trap has a call to ip_route(...) shown below. If
the IP address of the target platform does not match the trap destination, it
returns a null. For example, with netmask 255.255.255.0, if the target platform
is 192.168.0.2 and the trap destination is 192.168.1.5, ip_route(...) will
return null. Then the code will operate on a null pointer.

I think the call should either return an error, set an err value, or use an
ASSERT, etc. Most people probably don't have a problem because they are on the
same subnet. I only ran into the problem because I did not know that the
ip_route() could not handle the more complex routing.

snmp_send_trap(s8_t generic_trap, struct snmp_obj_id *eoid, s32_t
specific_trap)
{
  struct snmp_trap_dst *td;
  struct netif *dst_if;
  struct ip_addr dst_ip;
  struct pbuf *p;
  u16_t i,tot_len;

  for (i=0, td = &trap_dst[0]; i<SNMP_TRAP_DESTINATIONS; i++, td++)
  {
    if ((td->enable != 0) && (td->dip.addr != 0))
    {
      /* network order trap destination */
      trap_msg.dip.addr = td->dip.addr;
      /* lookup current source address for this dst */
      dst_if = ip_route(&td->dip);
      dst_ip.addr = ntohl(dst_if->ip_addr.addr);

The next problem is an inconsistency in IP representation handling. This code
below works because it calls htonl(...) before snap_trap_dst_ip_set(...), which
has another htonl(...) call. Most of the lwip takes an address in the form
returned by IP4_ADDR(...). This inconsistency lead to some debug time trying to
figure out why it did not work.

struct ip_addr addr;
IP4_ADDR(&addr, 192, 168, 1, 102);
addr.addr = htonl(snmp_message->ADDRESS.addr);
snmp_trap_dst_ip_set(0,&addr);

Because this was tricky to make work the first time, I leave my code here for
the next person trying this. I have not tested if this leaks memory yet, so if
you use it, beware it might leak. The context is this code receives a message
from a mailbox in an lwip thread. I don't know if I really need the callback or
not. I found an example working in a similar way and used it. If nothing else
this gives the context of how I am using lwip when I have the above two
problems.

// Code in lwip thread

struct ip_addr addr;
addr.addr = htonl(snmp_message->ADDRESS.addr);
snmp_trap_dst_ip_set(0,&addr);
snmp_trap_dst_enable(0,snmp_message->TRAP);

struct snmp_varbind *vb = snmp_varbind_alloc(&objid, SNMP_ASN1_OPAQUE,
strlen(snmp_message->message));
struct snmp_varbind_root *vb_root = (struct snmp_varbind_root
*)malloc(sizeof(struct snmp_varbind_root));;
memset(vb_root, 0, sizeof(struct snmp_varbind_root));
memcpy (vb->value, snmp_message->message, strlen(snmp_message->message));
snmp_varbind_tail_add(vb_root,vb);
tcpip_callback(send_trap_callback, vb_root);

void send_trap_callback( void * parameters )
{

    struct snmp_varbind_root *vb_root;

    if( parameters != NULL )
    {
        vb_root = (struct snmp_varbind_root *) parameters;
        struct snmp_obj_id eoid;
        // there should be a way to do this directly.
        eoid.id[0] = enterprises_ids[1];
        eoid.len = 1;

        trap_msg.outvb = *vb_root;

        snmp_send_trap(SNMP_GENTRAP_ENTERPRISESPC, &eoid, 1);

        // Need to know what should be freed.
        // message text?
        // struct snmp_varbind?
        // struct snmp_varbind_root?
    }
}

-- 
You are receiving this mail because:
You are on the CC list for the bug.