buglwIP - A Lightweight TCP/IP stack - Bugs: bug #51520, Big endian bug in...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #51520: Big endian bug in apps/snmp/snmp_asn1.c

Submitter:  Art Heers <artheers>
Submitted:  Thu 20 Jul 2017 09:32:57 PM UTC
   
 
Category:  apps Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  None
lwIP version:  2.0.2

Jump to the original submission

Wed 26 Jul 2017 10:56:43 AM UTC, comment #8: 

Fixed using Art's patch

Dirk Ziegelmeier <dziegel>
Group administrator
Wed 26 Jul 2017 07:31:24 AM UTC, comment #7: 

Pushed Art's patch
Wait for feedback on COUNTER64 before closing this - I'm sure it'll be negative, because pointer++ on an u32_t* won't work for 64 bit values on big endian machines :-)

Dirk Ziegelmeier <dziegel>
Group administrator
Wed 26 Jul 2017 06:50:27 AM UTC, comment #6: 

Thanks for the patch, we'll test it.

While reading the code, I found that COUNTER64 probably doesn't work for big endian, too. However, this is unimplemented yet, so not easy to test.

If you find the time, I'd be happy if you could test COUNTER64 by applying this patch:

(file #41338)

Simon Goldschmidt <goldsimon>
Group administrator
Tue 25 Jul 2017 10:56:56 PM UTC, comment #5: 

A patch with no reference to byte order: tested on a big endian machine.

/**

  • Decodes integer into s32_t.

 *

  • @param pbuf_stream points to a pbuf stream
  • @param len length of the coded integer field
  • @param value return host order integer
  • @return ERR_OK if successful, ERR_ARG if we can't (or won't) decode

 *

  • @note ASN coded integers are always signed!

 */
err_t
snmp_asn1_dec_s32t(struct snmp_pbuf_stream *pbuf_stream, u16_t len, s32_t *value)
{
  u8_t data;

  if ((len > 0) && (len < 5)) {
    PBUF_OP_EXEC(snmp_pbuf_stream_read(pbuf_stream, &data));

    if (data & 0x80) {
      /* negative, start from -1 */
      *value = (-1 << 8) | data;
    } else {
      /* positive, start from 0 */
      *value = data;
    }
    len--;
    /* shift in the remaining value */
    while (len > 0) {
      PBUF_OP_EXEC(snmp_pbuf_stream_read(pbuf_stream, &data));
      *value = (*value << 8) | data;
      len--;
    }
    return ERR_OK;
  }

  return ERR_VAL;
}

Art Heers <artheers>
Tue 25 Jul 2017 08:17:01 PM UTC, comment #4: 

Art, can you please provide a patch that fixes this?

Dirk Ziegelmeier <dziegel>
Group administrator
Tue 25 Jul 2017 07:57:41 PM UTC, comment #3: 

I'd very much appreciate to have this function changed. There should be no dependencies to BYTE_ORDER unless absolutely necessary!

Simon Goldschmidt <goldsimon>
Group administrator
Tue 25 Jul 2017 04:59:34 PM UTC, comment #2: 

We (Campbell Scientific) are using the Renesas processors, most recently the RX series, Rx63N, in our measurement and control systems (dataloggers).  Over the years we have built up our own operating system and at some point a few years ago gravitated to the LwIP stack.  The Renesas processors (originally Hitachi) followed Motorola's big endian model, though their more recent processors can optionally be run as little also.  We have stuck to big endian.  Renesas does have a development platform that you might want to look into.

As for the code issue, the function could have been written in such a way to avoid checking BYTE_ORDER by simply calculating what value is locally and then passing that value back via the pointer at the bottom of the function.  Instead it chose to use a pointer to value and to use that pointer during the creation of the value, one byte at a time, and therefore had to know the byte order of the machine.  It would be better, like you suggest to leave out BYTE_ORDER altogether by using a local uint32 localvalue, calculate the value with it, and the at the end do *value = localvalue.

Art Heers <artheers>
Mon 24 Jul 2017 08:03:23 PM UTC, comment #1: 

I'd really love to get hold of a big endian test platform! Unfortunately, I only have access to little endian systems. Which platform are you using to check this?

From reading the code I get confused. Shouldn't the host system do the correct thing without checking BYTE_ORDER at all?

Simon Goldschmidt <goldsimon>
Group administrator
Thu 20 Jul 2017 09:32:57 PM UTC, original submission:  


In this function:
err_t
snmp_asn1_dec_s32t(struct snmp_pbuf_stream *pbuf_stream, u16_t len, s32_t *value)


this code is wrong for BIG_ENDIAN machines:

#if BYTE_ORDER == LITTLE_ENDIAN
      *value <<= 8;
#endif
#if BYTE_ORDER == BIG_ENDIAN
      *value >>= 8;
#endif


For both little and big endian, it should be *value <<= 8;

For big endian machines, *value always ends up with only the least significant byte.


Art Heers <artheers>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #41338:  mib2_COUNTER64.patch added by goldsimon (2KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dziegel (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by artheers (Submitted the item)
  • -email is unavailable- added by artheers (snmp big endian bug)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-07-26 dziegel StatusNone Fixed
        Open/ClosedOpen Closed
    2017-07-26 goldsimon Attached File- Added mib2_COUNTER64.patch, #41338
    2017-07-20 artheers Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code