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

Peter Kästle peter.kaestle at nokia.com
Mon Nov 11 08:08:49 UTC 2024


On 08.11.24 23:29, David Laight wrote:
> From: Peter Kästle
>> Sent: 08 November 2024 18:12
>>
>> 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?
> ...
> 
> You might need to write them :-)
> 
> But something like:
>          ((struct { unsigned short x __attribute__((packed)); } *)bp)->x
> might DTRT.

I still think this is really a good idea, but I'd see it as a different 
patchset as it's an improvement and not really part of this bug fix 
proposal.  Furthermore other occurrences of move_from_unaligned* outside 
of display() F_INT: and F_UINT: could be improved by that.

Could you please review the patch of my original mail, whether it could 
be taken in use as fix of the actual problem?

best regards,
--peter;


More information about the busybox mailing list