Libguestfs - Feb 2017 - Re: [PATCH v2 1/2] lib: change how hbin sections are read.

On Wed, 2017-02-15 at 16:50 -0500, Dawid Zamirski wrote:
> On Wed, 2017-02-15 at 21:14 +0000, Richard W.M. Jones wrote:
> > On Wed, Feb 15, 2017 at 02:28:41PM -0500, Dawid Zamirski wrote:
> > > Correct, however there's also no guarantee that seeking by 4k
in
> > > "garbage" data would not land you in registry data that
happens
> > > to
> > > evaluate to "hbin" as well. That's why I put
"hbin" offset
> > > validation
> > > check couple of lines below to make sure that the
"hbin" we found
> > > by
> > > searching is a proper one. The offset check I'm referring to
is:
> > > 
> > > /* get "stated" hbin offset from header */
> > > size_t page_offset = le32to(page->offset_first) + 0x1000;
> > > 
> > > /* if that does not match our current file offset,
> > >    then exit with error */
> > > if (page_offset != off) { 
> > >   SET_ERRNO...
> > > }
> > 
> > Still, what kind of corruption would move a genuine hbin to a
> > non-page-sized offset in the file?  It seems unlikely to me ...
> > 
> > Rich.
> > 
> 
> None that I know of :-) However I don't see any other simple way to
> silence that GCC7 warning and it seems harmless that way to me
> (besides
> the fact that the loop will have to make more iterations)
So I've found a way to keep offsetting by 4k and keep GCC7 happy:

while (off < h->endpages) {
  if (off + 0x1000 > off)
    off += 0x1000;
  else
    break; // off would overflow

Is this acceptable?

Regards,
Dawid

On Wed, Feb 15, 2017 at 05:22:52PM -0500, Dawid Zamirski
wrote:
> On Wed, 2017-02-15 at 16:50 -0500, Dawid Zamirski wrote:
> > On Wed, 2017-02-15 at 21:14 +0000, Richard W.M. Jones wrote:
> > > On Wed, Feb 15, 2017 at 02:28:41PM -0500, Dawid Zamirski wrote:
> > > > Correct, however there's also no guarantee that seeking
by 4k in
> > > > "garbage" data would not land you in registry data
that happens
> > > > to
> > > > evaluate to "hbin" as well. That's why I put
"hbin" offset
> > > > validation
> > > > check couple of lines below to make sure that the
"hbin" we found
> > > > by
> > > > searching is a proper one. The offset check I'm
referring to is:
> > > > 
> > > > /* get "stated" hbin offset from header */
> > > > size_t page_offset = le32to(page->offset_first) + 0x1000;
> > > > 
> > > > /* if that does not match our current file offset,
> > > >    then exit with error */
> > > > if (page_offset != off) { 
> > > >   SET_ERRNO...
> > > > }
> > > 
> > > Still, what kind of corruption would move a genuine hbin to a
> > > non-page-sized offset in the file?  It seems unlikely to me ...
> > > 
> > > Rich.
> > > 
> > 
> > None that I know of :-) However I don't see any other simple way
to
> > silence that GCC7 warning and it seems harmless that way to me
> > (besides
> > the fact that the loop will have to make more iterations)
> 
> So I've found a way to keep offsetting by 4k and keep GCC7 happy:
> 
> while (off < h->endpages) {
>   if (off + 0x1000 > off)
>     off += 0x1000;
>   else
>     break; // off would overflow
> 
> Is this acceptable?
Yes, or even how about this (not tried it):

while (off <= h->endpages - 0x1000) {
  ...
}

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Wed, Feb 15, 2017 at 10:29:41PM +0000, Richard W.M. Jones
wrote:
> Yes, or even how about this (not tried it):
> 
> while (off <= h->endpages - 0x1000) {
>   ...
> }
In fact this doesn't work either :-(

I'll have another look at this tomorrow morning.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top