Xen devel - May 2012 - [PATCH] make pygrub cope better with big files in guest

Pygrub can use a lot of memory if the kernel or ramdisk files in a guest 
are very big as it reads them into memory before writing them out again to 
temporary files (these can legitimately be big for example the initrd.img 
file in a Fedora 16 install is around 130MB ).

This patch allows these files to be copied in one megabyte pieces, and if 
it sees any write problems it delets the files and exits. It also only 
reads up to the first megabyte of configurations files for grub etc. to 
avoid problems here as well (as it is a text file it should actually be 
much smaller).

This issue was reported by Xinli Niu in the Fedora bug report
https://bugzilla.redhat.com/show_bug.cgi?id=818412 who got it a CVE 
reference CVE-2012-2625 .

 	Michael Young

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

On Wed, 16 May 2012, M A Young wrote:
> Pygrub can use a lot of memory if the kernel or ramdisk files in a guest
are
> very big as it reads them into memory before writing them out again to 
> temporary files (these can legitimately be big for example the initrd.img 
> file in a Fedora 16 install is around 130MB ).
>
> This patch allows these files to be copied in one megabyte pieces, and if
it
> sees any write problems it delets the files and exits. It also only reads
up
> to the first megabyte of configurations files for grub etc. to avoid
problems
> here as well (as it is a text file it should actually be much smaller).
>
> This issue was reported by Xinli Niu in the Fedora bug report
> https://bugzilla.redhat.com/show_bug.cgi?id=818412 who got it a CVE
reference
> CVE-2012-2625 .
I realized the first patch was flawed as I was potentially using a file 
descriptor after I closed it. This is an untested correction.

 	Michael Young

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

On Wed, 2012-05-16 at 08:25 +0100, M A Young wrote:
> On Wed, 16 May 2012, M A Young wrote:
> 
> > Pygrub can use a lot of memory if the kernel or ramdisk files in a
guest are
> > very big as it reads them into memory before writing them out again to
> > temporary files (these can legitimately be big for example the
initrd.img
> > file in a Fedora 16 install is around 130MB ).
> >
> > This patch allows these files to be copied in one megabyte pieces, and
if it
> > sees any write problems it delets the files and exits. It also only
reads up
> > to the first megabyte of configurations files for grub etc. to avoid
problems
> > here as well (as it is a text file it should actually be much
smaller).
> >
> > This issue was reported by Xinli Niu in the Fedora bug report
> > https://bugzilla.redhat.com/show_bug.cgi?id=818412 who got it a CVE
reference
> > CVE-2012-2625 .
> 
> I realized the first patch was flawed as I was potentially using a file 
> descriptor after I closed it. This is an untested correction.
I think you might want a couple of closes on datafile in here. Otherwise
it looks good to me (by inspection, I''ve not run it either).

Thanks!

Ian.

On Wed, 16 May 2012, Ian Campbell wrote:
> On Wed, 2012-05-16 at 08:25 +0100, M A Young wrote:
>> On Wed, 16 May 2012, M A Young wrote:
>>
>>> Pygrub can use a lot of memory if the kernel or ramdisk files in a
guest are
>>> very big as it reads them into memory before writing them out again
to
>>> temporary files (these can legitimately be big for example the
initrd.img
>>> file in a Fedora 16 install is around 130MB ).
>>>
>>> This patch allows these files to be copied in one megabyte pieces,
and if it
>>> sees any write problems it delets the files and exits. It also only
reads up
>>> to the first megabyte of configurations files for grub etc. to
avoid problems
>>> here as well (as it is a text file it should actually be much
smaller).
>>>
>>> This issue was reported by Xinli Niu in the Fedora bug report
>>> https://bugzilla.redhat.com/show_bug.cgi?id=818412 who got it a CVE
reference
>>> CVE-2012-2625 .
>>
>> I realized the first patch was flawed as I was potentially using a file
>> descriptor after I closed it. This is an untested correction.
>
> I think you might want a couple of closes on datafile in here. Otherwise
> it looks good to me (by inspection, I''ve not run it either).
Yes, that sounds like a good idea. I did test the first version of the 
patch for basic functionality, though not how well it would handle 
problem situations.

 	Michael Young

On Wed, 16 May 2012, Ian Campbell wrote:
> On Wed, 2012-05-16 at 08:25 +0100, M A Young wrote:
>> On Wed, 16 May 2012, M A Young wrote:
>>
>>> Pygrub can use a lot of memory if the kernel or ramdisk files in a
guest are
>>> very big as it reads them into memory before writing them out again
to
>>> temporary files (these can legitimately be big for example the
initrd.img
>>> file in a Fedora 16 install is around 130MB ).
>>>
>>> This patch allows these files to be copied in one megabyte pieces,
and if it
>>> sees any write problems it delets the files and exits. It also only
reads up
>>> to the first megabyte of configurations files for grub etc. to
avoid problems
>>> here as well (as it is a text file it should actually be much
smaller).
>>>
>>> This issue was reported by Xinli Niu in the Fedora bug report
>>> https://bugzilla.redhat.com/show_bug.cgi?id=818412 who got it a CVE
reference
>>> CVE-2012-2625 .
>>
>> I realized the first patch was flawed as I was potentially using a file
>> descriptor after I closed it. This is an untested correction.
>
> I think you might want a couple of closes on datafile in here. Otherwise
> it looks good to me (by inspection, I''ve not run it either).
I am attaching the third version of this patch. The datafile object 
doesn''t have an explicit close so this patch uses del to clean it. I
was
also using unlink incorrectly which is now fixed.

I have tested it for basic functionality and also used pygrub directly to 
write to a very small file system and with this patch pygrub deletes the 
temporary files it creates if it runs out of file space.

 	Michael Young

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

M A Young writes ("Re: [Xen-devel] [PATCH] make pygrub cope better with big
files in guest"):
> I am attaching the third version of this patch. The datafile object 
> doesn''t have an explicit close so this patch uses del to clean it.
I was
> also using unlink incorrectly which is now fixed.
Why does pygrub need to unlink the output files on error ?  Shouldn''t
pygrub''s caller do that ?  libxl does.

Also the two bits of code which read the kernel and ramdisk are now
much larger and very similar to each other.  Couldn''t this be
refactored to have less duplication ?

Python even supports local functions...

Ian.

Ian Jackson writes ("Re: [Xen-devel] [PATCH] make pygrub cope better with
big files in guest"):
> Also the two bits of code which read the kernel and ramdisk are now
> much larger and very similar to each other.  Couldn''t this be
> refactored to have less duplication ?
And, would shutil.copyfileobj be of any use ?  It goes back as far as
Python 2.3 at least.

Ian.

On Tue, 2012-05-29 at 12:30 +0100, Ian Jackson wrote:
> Ian Jackson writes ("Re: [Xen-devel] [PATCH] make pygrub cope better
with big files in guest"):
> > Also the two bits of code which read the kernel and ramdisk are now
> > much larger and very similar to each other.  Couldn''t this be
> > refactored to have less duplication ?
> 
> And, would shutil.copyfileobj be of any use ?  It goes back as far as
> Python 2.3 at least.
I think, although I''m not sure, that the objects which libfsimage gives
out aren''t actually File''s in the python type sense.

Ian.

On Tue, 29 May 2012, Ian Jackson wrote:
> M A Young writes ("Re: [Xen-devel] [PATCH] make pygrub cope better
with big files in guest"):
>> I am attaching the third version of this patch. The datafile object
>> doesn''t have an explicit close so this patch uses del to clean
it. I was
>> also using unlink incorrectly which is now fixed.
>
> Why does pygrub need to unlink the output files on error ? 
Shouldn''t
> pygrub''s caller do that ?  libxl does.
We were having a similar discussion on the thread about a rival patch 
http://lists.xen.org/archives/html/xen-devel/2012-05/msg01499.html
I was playing it safe and removing the files quickly when an error occurs, 
possibly due to one of these files taking the available space. If the 
calling process will clear up properly and isn''t going to be confused
by
incomplete files then the unlinking isn''t needed.
> Also the two bits of code which read the kernel and ramdisk are now
> much larger and very similar to each other.  Couldn''t this be
> refactored to have less duplication ?
The rival patch mentioned above does this, so that might be a reasonable 
alternative, though it doesn''t handle the case of a huge grub (or
similar)
configuration file.

On Tue, 2012-05-29 at 12:30 +0100, Ian Jackson wrote:
> And, would shutil.copyfileobj be of any use ?  It goes back as far as
> Python 2.3 at least.
It might do. I suspect that the fsimage is sufficiently like a file for 
that to work, though it would need to be tested.

 	Michael Young

On Tue, 29 May 2012, M A Young wrote:
> On Tue, 2012-05-29 at 12:30 +0100, Ian Jackson wrote:
>> And, would shutil.copyfileobj be of any use ?  It goes back as far as
>> Python 2.3 at least.
>
> It might do. I suspect that the fsimage is sufficiently like a file for
that
> to work, though it would need to be tested.
I did some testing and it isn''t sufficiently like a file, I think
because
it doesn''t have a file pointer so it just reads the same bytes over and
over again.

 	Michael Young