[PATCH] libbb/dump: correct handling of unsigned on big endian

Peter Kästle peter.kaestle at nokia.com
Fri Nov 8 18:11:32 UTC 2024


On 08.11.24 15:38, David Laight wrote:
> From: Peter Kaestle
>> Sent: 08 November 2024 14:02
>>
>> The following commit broke dumping unsigned on big endian:
>> 34751d8bf (libbb/dump: correct handling of 1-byte signed int format, 2023-05-26)
>>
>> Example of the bug:
>>
>> root at fct:~ >echo -ne "\xfe\xfd\xfc\xfb" > /tmp/testfile
>>
>> before 34751d8bf:
>> $>/tmp/busybox_1_36_1 hexdump /tmp/testfile
>> 0000000 fefd fcfb
>> 0000004
>>
>> with 34751d8bf:
>> $>busybox_1_37_0 hexdump /tmp/testfile
>> 0000000 fefd00fe fcfb00fc
>> 0000004
>>
>> Problem originated from loading data into 32-bit "value" variable and
>> then doing the memcpy of 2 or 4 bytes into value afterwards, without
>> taking care of the byte order in case of big-endian cpus.
>>
>> Replicating same solution used in F_INT with the union fixes the issue.
> 
> I'd suggest getting rid of move_from_unaligned16/32() and replacing
> with something that just returns the value.
> Then the clauses are just:
>          case 2:
>                  value = read_unaligned_u16(bp);
>                  break;
> and nothing will go wrong.

Good idea, but unfortunately after "git grepping" busybox repo, it seems 
these kind of functions are not defined anywhere.
All I can find is those "move_from_unaligned*" functions defined inside 
include/platform.h.  And some "get_unaligned_be32/get_unaligned_le32", 
which are obviously not endian-safe.

Could you please help me understand, where you think those functions 
should come from?

But the fact, that Denys originally introduced those 
"move_from_unaligned" calls at this place with 34751d8bf (libbb/dump: 
correct handling of 1-byte signed int format, 2023-05-26) is an 
indication to me, that this is the right way to do it.
He probably just overlooked the issue with big-endian, described in my 
original post.

best regards,
--peter;


> The code will be much smaller as well.
> on x86 the above is just *(unsigned short *)bp rather than something
> that (I suspect) call memcpy().
> 
>          David
> 
> 
>>
>> Signed-off-by: Peter Kaestle <peter.kaestle at nokia.com>
>> ---
>>   libbb/dump.c | 12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/libbb/dump.c b/libbb/dump.c
>> index b406a2428..87180dff5 100644
>> --- a/libbb/dump.c
>> +++ b/libbb/dump.c
>> @@ -665,15 +665,23 @@ static NOINLINE void display(priv_dumper_t* dumper)
>>                                                        conv_u(pr, bp);
>>                                                        break;
>>                                                case F_UINT: {
>> +                                                     union {
>> +                                                             uint16_t val16;
>> +                                                             uint32_t val32;
>> +                                                             uint64_t val64;
>> +                                                     } u;
>>                                                        unsigned value = (unsigned char)*bp;
>> +
>>                                                        switch (pr->bcnt) {
>>                                                        case 1:
>>                                                                break;
>>                                                        case 2:
>> -                                                             move_from_unaligned16(value, bp);
>> +                                                             move_from_unaligned16(u.val16, bp);
>> +                                                             value = u.val16;
>>                                                                break;
>>                                                        case 4:
>> -                                                             move_from_unaligned32(value, bp);
>> +                                                             move_from_unaligned32(u.val32, bp);
>> +                                                             value = u.val32;
>>                                                                break;
>>                                                        /* case 8: no users yet */
>>                                                        }
>> --
>> 2.35.7
>>
>> _______________________________________________
>> busybox mailing list
>> busybox at busybox.net
>> https://lists.busybox.net/mailman/listinfo/busybox
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 



More information about the busybox mailing list