Re: [PATCH] libbb/dump: correct handling of unsigned on big endian
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@fct:~ >echo -ne "\xfe\xfd\xfc\xfb" > /tmp/testfile
before 34751d8bf:
$>/tmp/busybox_1_36_1 hexdump /tmp/testfile
000 fefd fcfb
004
with 34751d8bf:
$>busybox_1_37_0 hexdump /tmp/testfile
000 fefd00fe fcfb00fc
004
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;
___
busybox mailing list
busybox@busybox.net
https://lists.busybox.net/mailman/listinfo/busybox
RE: [PATCH] libbb/dump: correct handling of unsigned on big endian
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@fct:~ >echo -ne "\xfe\xfd\xfc\xfb" > /tmp/testfile
> >>
> >> before 34751d8bf:
> >> $>/tmp/busybox_1_36_1 hexdump /tmp/testfile
> >> 000 fefd fcfb
> >> 004
> >>
> >> with 34751d8bf:
> >> $>busybox_1_37_0 hexdump /tmp/testfile
> >> 000 fefd00fe fcfb00fc
> >> 004
> >>
> >> 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.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)
___
busybox mailing list
busybox@busybox.net
https://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] libbb/dump: correct handling of unsigned on big endian
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@fct:~ >echo -ne "\xfe\xfd\xfc\xfb" > /tmp/testfile
before 34751d8bf:
$>/tmp/busybox_1_36_1 hexdump /tmp/testfile
000 fefd fcfb
004
with 34751d8bf:
$>busybox_1_37_0 hexdump /tmp/testfile
000 fefd00fe fcfb00fc
004
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
---
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@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)
___
busybox mailing list
busybox@busybox.net
https://lists.busybox.net/mailman/listinfo/busybox
RE: [PATCH] libbb/dump: correct handling of unsigned on big endian
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
> 000 fefd fcfb
> 004
>
> with 34751d8bf:
> $>busybox_1_37_0 hexdump /tmp/testfile
> 000 fefd00fe fcfb00fc
> 004
>
> 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
> ---
> 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@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)
___
busybox mailing list
busybox@busybox.net
https://lists.busybox.net/mailman/listinfo/busybox
