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@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.

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 <[email protected]>
> ---
>  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
> [email protected]
> https://lists.busybox.net/mailman/listinfo/busybox

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, 
UK
Registration No: 1397386 (Wales)

_______________________________________________
busybox mailing list
[email protected]
https://lists.busybox.net/mailman/listinfo/busybox

Reply via email to