Re: [Xen-devel] [PATCH 10/16] libelf: check all pointer accesses

[George Dunlap <dunlapg@xxxxxxxxx>]

On Tue, Jun 4, 2013 at 6:59 PM, Ian Jackson <ian.jackson@xxxxxxxxxxxxx> wrote:
>  * ELF_ADVANCE_DEST is now safe in the sense that you can use it to
>    chop parts off the front of the dest area but if you chop more than
>    is available, the dest area is simply set to be empty, preventing
>    future accesses.

[snip]

> -#define ELF_ADVANCE_DEST(elf, amount)  elf->dest += (amount)
> -  /* Advances past amount bytes of the current destination area. */
> +/* Advances past amount bytes of the current destination area. */
> +static inline void ELF_ADVANCE_DEST(struct elf_binary *elf, uint64_t amount)
> +{
> +    if ( elf->dest_size >= amount )
> +    {
> +        elf->dest_base += amount;
> +        elf->dest_size -= amount;
> +    }
> +    else
> +    {
> +        elf->dest_size = 0;
> +    }
> +}

This is probably a minor thing, but is there a reason not to set
elf->dest_base to NULL here?

I realize that technically you've said "dest_size=0 -> dest_base is
also invalid", but it never hurts to have a little extra safety.

Other than that, I've tried to do a careful reading, and it seems like
it does what it says on the tin.  I have mostly looked at the code
that *has* changed, not the code that has *not* changed, but the
renaming of key structures and functions should have helped the
compiler do that for us.  It's good to go in with or without the above
suggestion as far as I can tell.

Reviewed-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel