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

Submitted by:  Art Heers <artheers>
Submitted on:  Thu 20 Jul 2017 09:32:57 PM UTC  
 
Category: appsSeverity: 3 - Normal
Item Group: Faulty BehaviourStatus: Fixed
Privacy: PublicAssigned to: None
Open/Closed: ClosedPlanned Release: None
lwIP version: 2.0.2

(Jump to the original submission Jump to the original submission)

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

Fixed using Art's patch

Dirk Ziegelmeier <dziegel>
Project Member
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>
Project Member
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>
Project 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>
Project Member
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>
Project 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>
Project 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>

 

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
  • -unavailable- added by dziegel (Posted a comment)
  • -unavailable- added by goldsimon (Posted a comment)
  • -unavailable- added by artheers (Submitted the item)
  • -unavailable- added by artheers (snmp big endian bug)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 4 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 26 Jul 2017 10:56:43 AM UTCdziegelStatusNone=>Fixed
      Open/ClosedOpen=>Closed
    Wed 26 Jul 2017 06:50:27 AM UTCgoldsimonAttached File-=>Added mib2_COUNTER64.patch, #41338
    Thu 20 Jul 2017 09:32:57 PM UTCartheersCarbon-Copy-=>Added -unavailable-

    Back to the top


    Powered by Savane 3.1-cleanup1