Re: [Emacs-diffs] /srv/bzr/emacs/trunk r107264: shr.el (shr-rescale-image): Allow viewing large images.

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r107264: shr.el (shr-rescale-image): Allow viewing large images.

[Stefan Monnier]

>  (defun shr-rescale-image (data)
> -  (let ((image (create-image data nil t :ascent 100)))
> +  (let* ((max-image-size 0)
> +	 (image (create-image data nil t :ascent 100)))

The whole purpose of max-image-size was to avoid crashes and
denial-of-service, for example when viewing incorrectly-labeled images.
So it's especially important for SHR which will display images
downloaded from anywhere.


        Stefan

Re: /srv/bzr/emacs/trunk r107264: shr.el (shr-rescale-image): Allow viewing large images.

[Andy Moreton]

On Mon 13 Feb 2012, Stefan Monnier wrote:

>>  (defun shr-rescale-image (data)
>> -  (let ((image (create-image data nil t :ascent 100)))
>> +  (let* ((max-image-size 0)
>> +	 (image (create-image data nil t :ascent 100)))
>
> The whole purpose of max-image-size was to avoid crashes and
> denial-of-service, for example when viewing incorrectly-labeled images.
> So it's especially important for SHR which will display images
> downloaded from anywhere.
>
It would help that porpose if it had some documentation:

max-image-size's value is 6.0

Documentation:
Not documented as a variable.

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r107264: shr.el (shr-rescale-image): Allow viewing large images.

[Lars Ingebrigtsen]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> The whole purpose of max-image-size was to avoid crashes and
> denial-of-service, for example when viewing incorrectly-labeled images.
> So it's especially important for SHR which will display images
> downloaded from anywhere.

shr.el (if compiled with imagemagick support) will (by default) rescale
the images down.  But to do that, it goes through the display engine,
anyway, so it's an issue anyway.

But what crashes and denials of service does `create-image' on a large
image result in?

-- 
(domestic pets only, the antidote for overdose, milk.)
  http://lars.ingebrigtsen.no  *  Sent from my Rome

Re: /srv/bzr/emacs/trunk r107264: shr.el (shr-rescale-image): Allow viewing large images.

[Lars Ingebrigtsen]

Andy Moreton <andrewjmoreton@gmail.com> writes:

> It would help that porpose if it had some documentation:
>
> max-image-size's value is 6.0
>
> Documentation:
> Not documented as a variable.

How odd.  I could have sworn that that variable had a doc string
yesterday.

...

And now it does again!  I tapped `C-h v' on that variable, and got what
you got.  But now when I try again I get this:

max-image-size is a variable defined in `C source code'.
Its value is 6.0

Documentation:
Maximum size of images.
Emacs will not load an image into memory if its pixel width or
pixel height exceeds this limit.

If the value is an integer, it directly specifies the maximum
image height and width, measured in pixels.  If it is a floating
point number, it specifies the maximum image height and width
as a ratio to the frame height and width.  If the value is
non-numeric, there is no explicit limit on the size of images.


-- 
(domestic pets only, the antidote for overdose, milk.)
  http://lars.ingebrigtsen.no  *  Sent from my Rome

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r107264: shr.el (shr-rescale-image): Allow viewing large images.

[Paul Eggert]

On 02/13/2012 12:01 PM, Lars Ingebrigtsen wrote:
> what crashes and denials of service does `create-image' on a large
> image result in?

For merely large images, Emacs won't fit into RAM
and will start swapping like mad, resulting in denial
of service that makes it essentially useless.

For incredibly large images, the underlying libraries are
likely to have unchecked integer or memory overflows, dumping
core if you're lucky and having "interesting" behavior otherwise.
Even if the libraries themselves are reliable (a big "if"),
that part of Emacs is less well ironed out, and I wouldn't
be surprised if Emacs proper has exploitable bugs in this
area.

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r107264: shr.el (shr-rescale-image): Allow viewing large images.

[Lars Ingebrigtsen]

Paul Eggert <eggert@cs.ucla.edu> writes:

> For incredibly large images, the underlying libraries are
> likely to have unchecked integer or memory overflows, dumping
> core if you're lucky and having "interesting" behavior otherwise.
> Even if the libraries themselves are reliable (a big "if"),
> that part of Emacs is less well ironed out, and I wouldn't
> be surprised if Emacs proper has exploitable bugs in this
> area.

Right.  But the default value of 6.0 disallows a lot of real-world
images.  (9gag in particular.)  Perhaps binding it to, say, 60.0 in
shr.el would make sense?

-- 
(domestic pets only, the antidote for overdose, milk.)
  http://lars.ingebrigtsen.no  *  Sent from my Rome

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r107264: shr.el (shr-rescale-image): Allow viewing large images.

[Ted Zlatanov]

On Mon, 13 Feb 2012 21:33:02 +0100 Lars Ingebrigtsen <larsi@gnus.org> wrote: 

LI> Paul Eggert <eggert@cs.ucla.edu> writes:
>> For incredibly large images, the underlying libraries are
>> likely to have unchecked integer or memory overflows, dumping
>> core if you're lucky and having "interesting" behavior otherwise.
>> Even if the libraries themselves are reliable (a big "if"),
>> that part of Emacs is less well ironed out, and I wouldn't
>> be surprised if Emacs proper has exploitable bugs in this
>> area.

LI> Right.  But the default value of 6.0 disallows a lot of real-world
LI> images.  (9gag in particular.)  Perhaps binding it to, say, 60.0 in
LI> shr.el would make sense?

Or make `max-image-size' a plist of conditions, so filtering by absolute
size AND by relative size to frame AND by file size are all possible at
once?

Ted

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r107264: shr.el (shr-rescale-image): Allow viewing large images.

[Stefan Monnier]

>> The whole purpose of max-image-size was to avoid crashes and
>> denial-of-service, for example when viewing incorrectly-labeled images.
>> So it's especially important for SHR which will display images
>> downloaded from anywhere.
> shr.el (if compiled with imagemagick support) will (by default) rescale
> the images down.  But to do that, it goes through the display engine,
> anyway, so it's an issue anyway.
> But what crashes and denials of service does `create-image' on a large
> image result in?

We had a few bug reports: some incorrectly labeled images seemed to
indicate insanely large size, which resulted in crashes because malloc
failed, while other large images resulted in problems because of
allocation of very large blocks of memory for the image.
A quick search pointed me to the thread at
http://www.mail-archive.com/emacs-devel@gnu.org/msg10214.html
which doesn't show concrete bugs, but should give you some context.


        Stefan

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r107264: shr.el (shr-rescale-image): Allow viewing large images.

[Paul Eggert]

On 02/13/2012 12:33 PM, Lars Ingebrigtsen wrote:
> the default value of 6.0 disallows a lot of real-world
> images.  (9gag in particular.)  Perhaps binding it to, say, 60.0 in
> shr.el would make sense?

We have to do something.  The recently-changed trunk behavior,
where there's no limit at all, is asking for trouble.
It really needs to get fixed before Emacs 24 comes out.

By '9gag' do you mean 9gag.com?  Currently, its third image
<http://d24w6bsrhbeh9d.cloudfront.net/photo/2611347_460s.jpg>
is 472x4464, which I suppose could run into a problem with
max-image-size being 6.0 if your screen is small.

But 60.0 would be way too large.  A typical frame size for me
is 500x1100, and allowing 30,000x66,000 images would make my
5-year-old desktop (with 2 GiB of RAM) keel over and die.

I suggest the following heuristic instead.  Take the total amount
of physical memory, divide by 64, and reject images
that would consume more than that amount of RAM.
For example, my desktop has 2 GiB of physical RAM, so any image
requiring more than 32 MiB of RAM would be rejected;
this would easily allow the 9gag image mentioned above,
which consumes about 9 MiB assuming 32 bits per pixel.
The divisor "64" is a heuristic that could be user-adjusted;
but the point is that dividing RAM by 64 is a more-useful default
than multiplying the frame size by 6.

If all this is too much trouble, I suggest going back to the 6.0
limit for now, and revisiting this issue after Emacs 24.1 comes out.

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r107264: shr.el (shr-rescale-image): Allow viewing large images.

[Lars Ingebrigtsen]

Paul Eggert <eggert@cs.ucla.edu> writes:

> By '9gag' do you mean 9gag.com?

Yup.

> Currently, its third image
> <http://d24w6bsrhbeh9d.cloudfront.net/photo/2611347_460s.jpg> is
> 472x4464, which I suppose could run into a problem with max-image-size
> being 6.0 if your screen is small.

Yeah, it's a laptop with a 1280x800 screen.  I'd guesstimate that about
one fifth of the images from 9gag don't display with the 6.0 default.

> I suggest the following heuristic instead.  Take the total amount
> of physical memory, divide by 64, and reject images
> that would consume more than that amount of RAM.
> For example, my desktop has 2 GiB of physical RAM, so any image
> requiring more than 32 MiB of RAM would be rejected;
> this would easily allow the 9gag image mentioned above,
> which consumes about 9 MiB assuming 32 bits per pixel.
> The divisor "64" is a heuristic that could be user-adjusted;
> but the point is that dividing RAM by 64 is a more-useful default
> than multiplying the frame size by 6.

I think that's probably a better solution than the frame size thing.
Would it affect animated gifs, though?  I mean, currently Emacs doesn't
balk at displaying a 1TB huge animated GIF (or something :-)...

> If all this is too much trouble, I suggest going back to the 6.0
> limit for now, and revisiting this issue after Emacs 24.1 comes out.

I think the limit should be set to something that gives as little
problems to real-world usage as realistic.  6.0 is too small, nil is
unresponsible, and 60.0 is probably too large.  10.0?

-- 
(domestic pets only, the antidote for overdose, milk.)
  http://lars.ingebrigtsen.no  *  Sent from my Rome

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r107264: shr.el (shr-rescale-image): Allow viewing large images.

[Paul Eggert]

On 02/14/2012 06:49 AM, Lars Ingebrigtsen wrote:
> I think the limit should be set to something that gives as little
> problems to real-world usage as realistic.  6.0 is too small, nil is
> unresponsible, and 60.0 is probably too large.  10.0?

10.0 would be fine, I think, not too large for older desktops with
big displays.  The current value of 6.0 was established in 2005,
and the typical ratio of RAM to display size for older machines
has probably grown by a factor of (10/6)**2 since then.

But it should be 10.0 everywhere, no?  Not just in Gnus.  That is,
shouldn't the change be something like this?

=== modified file 'lisp/gnus/shr.el'
--- lisp/gnus/shr.el	2012-02-13 11:25:56 +0000
+++ lisp/gnus/shr.el	2012-02-14 15:54:44 +0000
@@ -557,8 +557,7 @@
     (insert alt)))
 
 (defun shr-rescale-image (data)
-  (let* ((max-image-size nil)
-	 (image (create-image data nil t :ascent 100)))
+  (let ((image (create-image data nil t :ascent 100)))
     (if (or (not (fboundp 'imagemagick-types))
 	    (not (get-buffer-window (current-buffer))))
 	image

=== modified file 'src/image.c'
--- src/image.c	2012-02-07 03:46:18 +0000
+++ src/image.c	2012-02-14 15:54:07 +0000
@@ -976,7 +976,7 @@
 
 static void free_image (struct frame *f, struct image *img);
 
-#define MAX_IMAGE_SIZE 6.0
+#define MAX_IMAGE_SIZE 10.0
 /* Allocate and return a new image structure for image specification
    SPEC.  SPEC has a hash value of HASH.  */
 

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r107264: shr.el (shr-rescale-image): Allow viewing large images.

[Stefan Monnier]

> Right.  But the default value of 6.0 disallows a lot of real-world
> images.  (9gag in particular.)  Perhaps binding it to, say, 60.0 in
> shr.el would make sense?

For a full-screen frame on a modern display, we're talking 2M pixels for
the frame, so you're suggesting to place the limit at 60x60x2M = 7G
pixels... I'm afraid that's clearly in the "crash&|DoS" zone.

Maybe the limit should be expressed in megabytes or in total number of
pixels (rather than in only one dimension) and relative to the screen
size rather than the frame size.


        Stefan

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r107264: shr.el (shr-rescale-image): Allow viewing large images.

[Lars Ingebrigtsen]

Paul Eggert <eggert@cs.ucla.edu> writes:

>  (defun shr-rescale-image (data)
> -  (let* ((max-image-size nil)

[...]

> -#define MAX_IMAGE_SIZE 6.0
> +#define MAX_IMAGE_SIZE 10.0

I think this would be the best (minimal) change.  With 10.0, it pretty
much looks like all the 9gag images display for me on this machine.

As Stefan says, perhaps this limit should be reworked to be, er,
different (based on total pixels or total size), but we're too late in
the release cycle for that, aren't we?

-- 
(domestic pets only, the antidote for overdose, milk.)
  http://lars.ingebrigtsen.no  *  Sent from my Rome

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r107264: shr.el (shr-rescale-image): Allow viewing large images.

[Chong Yidong]

Ted Zlatanov <tzz@lifelogs.com> writes:

> Or make `max-image-size' a plist of conditions, so filtering by absolute
> size AND by relative size to frame AND by file size are all possible at
> once?

Let's not over-engineer this.

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r107264: shr.el (shr-rescale-image): Allow viewing large images.

[Chong Yidong]

Paul Eggert <eggert@cs.ucla.edu> writes:

> 10.0 would be fine, I think, not too large for older desktops with
> big displays.  The current value of 6.0 was established in 2005,
> and the typical ratio of RAM to display size for older machines
> has probably grown by a factor of (10/6)**2 since then.
>
> But it should be 10.0 everywhere, no?  Not just in Gnus.  That is,
> shouldn't the change be something like this?

Looks OK, please go ahead and make this change (and revert the one in
shr.el).

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r107264: shr.el (shr-rescale-image): Allow viewing large images.

[Chong Yidong]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> As Stefan says, perhaps this limit should be reworked to be, er,
> different (based on total pixels or total size), but we're too late in
> the release cycle for that, aren't we?

The variable already accepts integer values to specify maximum pixel
width/height.  If memory serves, the reason we put it as a multiple of
the frame height/width has to do with the idea that it's generally
useless to show an image much larger than the typical frame size (even
with scrolling).  The distinction doesn't seem hugely important in
practice.

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r107264: shr.el (shr-rescale-image): Allow viewing large images.

[Ted Zlatanov]

On Wed, 15 Feb 2012 14:26:26 +0800 Chong Yidong <cyd@gnu.org> wrote: 

CY> Ted Zlatanov <tzz@lifelogs.com> writes:
>> Or make `max-image-size' a plist of conditions, so filtering by absolute
>> size AND by relative size to frame AND by file size are all possible at
>> once?

CY> Let's not over-engineer this.

OK, then how about not loading the entire image into memory?  It seems
crazy that we're limiting the size of loaded images because they might
cause Emacs to run out of memory.

Ted