Re: [PATCH] unlzma: avoid UB shift

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

On Tue, Mar 24, 2026 at 5:27 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> Shifting signed quantities has restrictions. Since the wrapping macro of
> read_int() type-casts the result anyway, switch function return type as
> well as the local variable to the corresponding unsigned type.
>
> Reported-by: Kamil Frankowicz <kamil.frankowicz@xxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> We've inherited that code from Linux, and the same code still exists
> there. As I'm entirely uncertain whether they would even care, I'd prefer
> to not take the route of posting a patch against Linux first.
>
> --- a/xen/common/unlzma.c
> +++ b/xen/common/unlzma.c
> @@ -30,10 +30,10 @@
>
>  #include "decompress.h"
>
> -static long long __init read_int(unsigned char *ptr, int size)
> +static unsigned long long __init read_int(unsigned char *ptr, int size)

nit: Since we're touching read_int() anyway, would it make sense to also
tighten the helper's interface, i.e. make ptr const and use size_t for
size?

That would better match the actual usage: the buffer is only read from,
and size is really a byte count, usually coming from sizeof().

>  {
>         int i;
> -       long long ret = 0;
> +       unsigned long long ret = 0;
>
>         for (i = 0; i < size; i++)
>                 ret = (ret << 8) | ptr[size-i-1];

Separately, the loop could also be written without the temporary i, using
a simpler reverse traversal, for example:

while ( size )
    ret = (ret << 8) | ptr[--size];

>

Best regards,
Mykola