PIL 1.1.6 ImageFile.Parser destroys data for PngImagePlugin

[Nils Olaf de Reus]

I am experiencing inconvenience from the behaviour of ImageFile.py in
PIL 1.1.6 when attempting to read in a PNG image.

The method I see documented to retreive an image through ImageFile is by
invoking close() on the ImageFile.Parser, like this.

>>> import ImageFile
>>> import urllib
>>>
>>> f = urllib.urlopen('<url-path-to-png-file>')
>>>
>>> p = ImageFile.Parser()
>>>
>>> while True:
...     s = f.read(1024)
...     if not s:
...         break
...     p.feed(s)
...
>>> im = p.close()

The way ImageFile.Parser works, it creates an ImageFile._ParserFile to
act as a file, and calls Image.open() on that. So far so good, and this
results in a valid png image object.. for a very brief moment, because
in the 'finally' section immediately after, it calls close() on that
ImageFile._ParserFile.

The definition of close() in ImageFile._ParserFile is as follows:

    def close(self):
        self.data = self.offset = None

But PngImagePlugin does not copy data on opening - it just keeps a
reference to the source. So this just now destroyed the data on the
object that PngImagePlugin is still referring to as the location of its
IDAT chunks.. not a bright move, because we have now created a condition
where from the point of view of the PngImagePlugin, self.fp is an
ImageFile._ParserFile instance, and self.fp.data is None.

Which means that as soon as we try to do anything with that image,
PngImagePlugin is going to be firing read() instructions at its self.fp,
which in turn blindly trusts that its self.data supports __getitem__()..
and predictably, there is our Traceback:

>>> im.convert('RGB')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.5/site-packages/PIL/Image.py", line 653, in
convert
    self.load()
  File "/usr/lib/python2.5/site-packages/PIL/ImageFile.py", line 189, in
load
    s = read(self.decodermaxblock)
  File "/usr/lib/python2.5/site-packages/PIL/PngImagePlugin.py", line
365, in load_read
    return self.fp.read(bytes)
  File "/usr/lib/python2.5/site-packages/PIL/ImageFile.py", line 300, in
read
    data = self.data[pos:pos+bytes]
TypeError: 'NoneType' object is unsubscriptable


I am currently working around the issue by creating my own
ImageFile._ParserFile object that avoids having its data None'd.

>>> import ImageFile
>>> import urllib
>>>
>>> f = urllib.urlopen('<url-path-to-png-file>')
>>>
>>> p = ImageFile.Parser()
>>>
>>> while True:
...     s = f.read(1024)
...     if not s:
...         break
...     p.feed(s)
...
>>> virtualfile = ImageFile._ParserFile(p.data)
>>> im = Image.open(virtualfile)
>>> p.close()
>>> im.convert('RGB')
<Image.Image instance at 0x2b890f5cff80>


This works great -just need to keep the virtualfile around until I'm
really done with it- but obviously I am not happy having to use what is
supposed to be an internal from ImageFile.

>From where I stand, the sensible thing would be to eliminate these two
lines from ImageFile.Parser.close():

            finally:
                fp.close() # explicitly close the virtual file

..except that because of the comment, I must assume that there is a
reason to be doing this. Could someone please shed light on why it is
necessary to explicitly close fp there?

Kind regards,
  Nils Olaf de Reus


_______________________________________________
Image-SIG maillist  -  Image-SIG@...
http://mail.python.org/mailman/listinfo/image-sig

[Fredrik Lundh]

Thanks for the detailed analysis.  The fix in 1.1.7 is slightly
different from the one you propose:

    http://hg.effbot.org/pil-2009-raclette/changeset/fe4688f15fed/

Not sure why the code considers it important to close the file at that
point; I'll take another look at a look at the code and the history of
that file when I find the time.

</F>

On Wed, Nov 4, 2009 at 2:16 PM, Nils Olaf de Reus
<nils.de.reus@...> wrote:

> I am experiencing inconvenience from the behaviour of ImageFile.py in
> PIL 1.1.6 when attempting to read in a PNG image.
>
> The method I see documented to retreive an image through ImageFile is by
> invoking close() on the ImageFile.Parser, like this.
>
>>>> import ImageFile
>>>> import urllib
>>>>
>>>> f = urllib.urlopen('<url-path-to-png-file>')
>>>>
>>>> p = ImageFile.Parser()
>>>>
>>>> while True:
> ...     s = f.read(1024)
> ...     if not s:
> ...         break
> ...     p.feed(s)
> ...
>>>> im = p.close()
>
> The way ImageFile.Parser works, it creates an ImageFile._ParserFile to
> act as a file, and calls Image.open() on that. So far so good, and this
> results in a valid png image object.. for a very brief moment, because
> in the 'finally' section immediately after, it calls close() on that
> ImageFile._ParserFile.
>
> The definition of close() in ImageFile._ParserFile is as follows:
>
>    def close(self):
>        self.data = self.offset = None
>
> But PngImagePlugin does not copy data on opening - it just keeps a
> reference to the source. So this just now destroyed the data on the
> object that PngImagePlugin is still referring to as the location of its
> IDAT chunks.. not a bright move, because we have now created a condition
> where from the point of view of the PngImagePlugin, self.fp is an
> ImageFile._ParserFile instance, and self.fp.data is None.
>
> Which means that as soon as we try to do anything with that image,
> PngImagePlugin is going to be firing read() instructions at its self.fp,
> which in turn blindly trusts that its self.data supports __getitem__()..
> and predictably, there is our Traceback:
>
>>>> im.convert('RGB')
> Traceback (most recent call last):
>  File "<stdin>", line 1, in <module>
>  File "/usr/lib/python2.5/site-packages/PIL/Image.py", line 653, in
> convert
>    self.load()
>  File "/usr/lib/python2.5/site-packages/PIL/ImageFile.py", line 189, in
> load
>    s = read(self.decodermaxblock)
>  File "/usr/lib/python2.5/site-packages/PIL/PngImagePlugin.py", line
> 365, in load_read
>    return self.fp.read(bytes)
>  File "/usr/lib/python2.5/site-packages/PIL/ImageFile.py", line 300, in
> read
>    data = self.data[pos:pos+bytes]
> TypeError: 'NoneType' object is unsubscriptable
>
>
> I am currently working around the issue by creating my own
> ImageFile._ParserFile object that avoids having its data None'd.
>
>>>> import ImageFile
>>>> import urllib
>>>>
>>>> f = urllib.urlopen('<url-path-to-png-file>')
>>>>
>>>> p = ImageFile.Parser()
>>>>
>>>> while True:
> ...     s = f.read(1024)
> ...     if not s:
> ...         break
> ...     p.feed(s)
> ...
>>>> virtualfile = ImageFile._ParserFile(p.data)
>>>> im = Image.open(virtualfile)
>>>> p.close()
>>>> im.convert('RGB')
> <Image.Image instance at 0x2b890f5cff80>
>
>
> This works great -just need to keep the virtualfile around until I'm
> really done with it- but obviously I am not happy having to use what is
> supposed to be an internal from ImageFile.
>
> >From where I stand, the sensible thing would be to eliminate these two
> lines from ImageFile.Parser.close():
>
>            finally:
>                fp.close() # explicitly close the virtual file
>
> ..except that because of the comment, I must assume that there is a
> reason to be doing this. Could someone please shed light on why it is
> necessary to explicitly close fp there?
>
> Kind regards,
>  Nils Olaf de Reus
>
>
> _______________________________________________
> Image-SIG maillist  -  Image-SIG@...
> http://mail.python.org/mailman/listinfo/image-sig
>

_______________________________________________
Image-SIG maillist  -  Image-SIG@...
http://mail.python.org/mailman/listinfo/image-sig

[Alexey Borzenkov]

Hi Fredrik,

On Wed, Nov 4, 2009 at 6:43 PM, Fredrik Lundh <fredrik@...> wrote:
> Thanks for the detailed analysis.  The fix in 1.1.7 is slightly
> different from the one you propose:
>
>    http://hg.effbot.org/pil-2009-raclette/changeset/fe4688f15fed/
>
> Not sure why the code considers it important to close the file at that
> point; I'll take another look at a look at the code and the history of
> that file when I find the time.

I looked at your changeset, and wonder why do you call image.load() in
finally? If Image.open fails, then self.image.load will also fail,
masking the original exception with attribute error. I don't remember
what I posted back then, but my patch was a little different:

http://git.kitsu.ru/patched/pil.git?a=commitdiff;h=943d371a903bf2af8ebc6b3be908bcdd6f8d0964
_______________________________________________
Image-SIG maillist  -  Image-SIG@...
http://mail.python.org/mailman/listinfo/image-sig