le_read16, be_read16 can return incorrect values

[tsteven4]

On a twos complement little endian machine with 4 byte integers the following code

int16_t number{-30964};
qDebug() << le_read16(&number);

results in a positive number being returned.
34572

-30964 as a two's complement 16 bit integer is 0x870C.
34572 as a 32 bit signed integer is 0x0000870C.
-30964 as a two's complement 32 bit integer is 0xFFFF870C.

It seems like navilink/sbp, sbn and skytraq count on this error.

Note 4 byte integers are very common these days (e.g. LLP64, LP64).

The endian routines in util.cc can be implemented with templates from QtEndian.

[robertlipe]

Eep. Isn't it that int16 is a signed (sometimes) that's getting us into trouble more than the 4 byte ints? 4 byte ints have been our norm forever, while this was the kind of thing that probably got sucked up into one of our big refactorings. Isn't it hte sign extension on the widening of the int16_t to the return value that's bad here? I kept trying to remove endianness implementations because our module authors invariably got them wrong. There was a time we had to worry about cases of Moto (Dragonball was BE) PDA formatand PowerPC/Sparc hosts and all the combinations in them, but there shouldn't be a whole lot using BE these days. I kept a PowerPC Mac Mini running in the GPSBabel longer than was sensible just to test this stuff, but it's been a very long time now - certainly before Qt and std::endianness were otions. We implemented those a lot of different wrong ways over the years. We can go QtEndian or std::endian. Isn't our remaining usage mostly hypothetical, like big-endian exif or 68K-era marine units?

…

On Sat, Jul 23, 2022 at 9:50 AM tsteven4 ***@***.***> wrote: On a twos complement little endian machine with 4 byte integers the following code int16_t number{-30964}; qDebug() << le_read16(&number); results in a positive number being returned. 34572 -30964 as a two's complement 16 bit integer is 0x870C. 34572 as a 32 bit signed integer is 0x0000870C. -30964 as a two's complement 32 bit integer is 0xFFFF870C. It seems like navilink/sbp count on this error. Note 4 byte integers are very common these days (e.g. LLP64, LP64). The endian routines in util.cc can be implemented with templates from QtEndian. — Reply to this email directly, view it on GitHub <#894>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCSD33BTLADQGJ67P2OX4TVVQBCPANCNFSM54ODSBEA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[tsteven4]

The lack of sign extension in [bl]e_read16, with >2 byte ints, is the issue. Historically the "signed" versions [bl]e_read(16|32) are much older than unsigned versions [bl]e_readu(16|32) (~ dc2158394 in 2002 vs. ~ a1ed48e1e in 2008). Today, the problem is that all the binary formats were developed without sign extension in the "signed" versions, and in many cases relied on that fact. In many cases they should have used [bl]e_readu16 instead of [bl]e_read16, but either because of history or a format uncertainty they didn't. After the fact it is difficult to decide if each use should be signed or unsigned.

If we change le_read16 (and the rest similarly) to

signed int
le_read16(const void* ptr)
{
  return qFromLittleEndian<qint16>(ptr);
}

which does the sign extension, then we have many test case failures.

std:endian is c++20, and std::byteswap is c++23.