After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 78064 - Entering large dimensions in Scale Image causes fatal error
Entering large dimensions in Scale Image causes fatal error
Status: RESOLVED OBSOLETE
Product: GIMP
Classification: Other
Component: General
1.x
Other All
: Normal major
: Future
Assigned To: GIMP Bugs
GIMP Bugs
: 592838 (view as bug list)
Depends on: 21028
Blocks:
 
 
Reported: 2002-04-08 12:02 UTC by aschenke
Modified: 2018-05-24 10:42 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch to replace g_new with g_try_malloc in tile_manager_get (1.43 KB, patch)
2003-11-20 17:46 UTC, Dave Neary
none Details | Review

Description aschenke 2002-04-08 12:02:20 UTC
Entering large values (like 2000000) for both horizontal (X) and 
vertical (Y) ratios with percent mode in the Scale Image dialog 
causes a fatal bus error a short time after clicking OK. This occurred 
with a new, blank image window sized 256 pixels by 256 pixels.
Comment 1 Sven Neumann 2002-04-09 11:08:28 UTC
Well, most probably gimp just runs out of memory. The scale dialog
should have the same warning message as the file-new dialog in order
to avoid these kind of problems when users accidentally enter insanely
large values. Didn't we already have a bug-report for this feature
request ?!
Comment 2 aschenke 2002-04-09 13:18:57 UTC
Do you mean this report ("Warn user about enormous memory 

consumption")?



http://bugzilla.gnome.org/show_bug.cgi?id=21028



If so, it would appear to be the same problem but it is described 

more as a feature request (severity is listed as trivial) and didn't 

appear from the description to be about the same issue (which 

is why I didn't find it with my initial searches).



I feel that this issue is of "critical" severity as it actually killed 

GIMP with a fatal error. Of course you may feel the priority should 

be low since people don't normally scale their images by two 

million percent, but there may be some lower threshold that also 

causes a crash (which may vary between machines/

environments). I personally have entered "50" instead of "0.5" 

when using percent scaling; entering values such as 200 or 300 

by accident doesn't seem unlikely to me. I think having GIMP 

crash like that would upset most users, especially if they had 

unsaved work when they did that.



I'll leave it up to you experts to decide how you want to handle it, 

but I'd like to point out the earlier issue report was opened nearly 

one and a half years ago (August 2000) and has apparently not 

been resolved yet. 
Comment 3 Raphaël Quinet 2002-04-09 16:16:10 UTC
Yes, bug #21028 is probably the one that Sven was refering to.  That
one is not critical because the problem described there did not seem
to cause a crash.  This bug report does, so it can be marked as
critical and left open (with a dependency on the other one).

I agree with you regarding the fact that it could be easy to enter 50
instead of .50 by mistake, and this should not cause the whole
application to crash without warning.  Regarding the severity of this
bug report, its Priority may be set to "Low" if we think that it will
not happen very often, but the Severity should be "Critical" because
this causes a crash.

You mention that the earlier bug report was entered more than one and
a half year ago and is still open.  Well, the last comment says "we
can fix this for 1.2.3 if someone comes with a small and clean patch."
Apparently, nobody had enough spare time to look into that issue.  It
is obviously too late for 1.2.3 now, but maybe not for 1.2.4.  If you
have some coding skills or know someone who does, please feel free to
submit a patch and attach it to that bug report.  That would be really
nice, because it looks like all current developers and contributors
are busy with other things.
Comment 4 Alan Horkan 2003-07-23 18:37:31 UTC
Changes at the request of Dave Neary on the developer mailing list.  
I am changing many of the bugzilla reports that have not specified a target
milestone to Future milestone.  Hope that is acceptable.  
Comment 5 Tino Schwarze 2003-07-24 10:03:36 UTC
Maybe we should just show the "larg image warning"-dialog if the new 
image is above the threshold given in the preferences (but the
original was not)?
Comment 6 Tomas Mraz 2003-07-25 14:16:47 UTC
The threshold for the warning dialog should be automatically computed
based on tile cache size. No another pref needed.
This could be done after feature freeze.
Comment 7 Sven Neumann 2003-07-25 14:49:30 UTC
I don't agree, it should be a separate option so that you can set it
very high if you dislike the warning. And the bug-report is about a
crash. Since we need a new feature to fix it, it should be addressed
before freeze if possible.
Comment 8 Dave Neary 2003-10-21 12:05:44 UTC
Is there a quick fix for this that could be put in place without
adding a new option?

Dave.
Comment 9 Pedro Gimeno 2003-11-11 23:01:18 UTC
I can't reproduce this with gimp-1.3 on my system (Debian testing with
ext3fs). I can see a limit of 262144x262144 on the resulting image
size. Of course such an image does not fit in my system, but unlike
what is stated in the description of this bug report it takes very
long before it crashes with the following messages:

(lots of the following line):
(gimp-1.3:25149): Gimp-Base-WARNING **: cache: unable to find room for
a tile

(and finally this):

GLib-ERROR **: gmem.c:140: failed to allocate 4096 bytes
aborting...
gimp-1.3: terminated: Aborted

The trace of the "free -s 3" command shows that almost all available
RAM and swap space becomes used.

With gimp-1.2, however, I can reproduce the problem because it allows
for sizes > 256Kx256K (up to 16Mx16M). Using 256Kx256K makes gimp-1.2
not to abort almost immediately but behave similarly to gimp-1.3,
except that it shows the warning earlier, probably because of the
issues that gimp-1.2 had with the tile swap file's 2Gb limit.

Should it be considered a bug that an application can't handle an
out-of-memory condition? Is it a bug that (say) emacs aborts when
trying to load a file that is larger than the available space (RAM
plus swap plus disk)? (this is not about whether/how Emacs itself
handles the situation, just to make things more dramatic.)

Can someone reproduce the problem as reported? Can it be MacOS specific?
Comment 10 Sven Neumann 2003-11-12 00:10:27 UTC
Well, you reproduced the problem, what is your point?

An application should either handle the OOM condition gracefully or
(or probably even and) warn the user that such a problem is likely to
happen. Especially if it is easy to accidentally trigger it and
there's no way to cancel the operation.

We could attempt to handle the memory allocation problems by using
g_try_malloc() in a few central places where large chunks or memory
are allocated.

But I think we should also add a user warning. We already have a size
limit for such warnings on the Image->New dialog. This value could be
used by other operations as well. It would probably be nice (and
trivial thanks to the prop widgets) to include the configuration for
the size limit in the warning dialog. See how the tips dialog
implements the "Show Tips next time" toggle so see how easy this can
be achieved.
Comment 11 Nathan Summers 2003-11-12 22:20:38 UTC
yes, just aborting is bad.  Very bad.  Data loss bad.  :)
Comment 12 Dave Neary 2003-11-13 20:53:03 UTC
Some information on this (I don't think I'll have time to delve much
further this weekend, so if someone else can have a go, that would be
cool).

I interrupted a session during the scale in gdb, and obtained the
following stack trace (snipping unimportant bits):
  • #0 mallopt
    from /lib/libc.so.6
  • #1 malloc
    from /lib/libc.so.6
  • #2 g_malloc
    from /usr/lib/libglib-2.0.so.0
  • #3 tile_manager_get
    at tile-manager.c line 180
  • #4 write_pixel_data
    at tile-manager.c line 805
  • #5 pixel_region_set_row
    at pixel-region.c line 172
  • #6 scale_region
    at paint-funcs.c line 3215

The important part here is actually in tile_manager_get. In here we
have tm, an unallocated TileManager *, whose value is this:
(gdb) print *tm
$1 = {ref_count = 1, x = 0, y = 0, width = 262144, height = 262144,
bpp = 1,
  ntile_rows = 4096, ntile_cols = 4096, tiles = 0x41a8f008,
validate_proc = 0,
  cached_num = -1, cached_tile = 0x0, user_data = 0x0}

Notice ntile_cols == ntile_rows == 4096.
We request *lots* of memory in this function - notably in this loop
here...

176           for (i = 0, k = 0; i < nrows; i++)
177             {
178               for (j = 0; j < ncols; j++, k++)
179                 {
180                   tiles[k] = g_new (Tile, 1);
181                   tile_init (tiles[k], tm->bpp);
182                   tile_attach (tiles[k], tm, k);
183
184                   if (j == (ncols - 1))
185                     tiles[k]->ewidth = right_tile;
186
187                   if (i == (nrows - 1))
188                     tiles[k]->eheight = bottom_tile;
189                 }
190             }


This is where we run out of memory and choke on our own vomit. The
correct (?) thing to do is to use a recoverable method of allocating
memory instead of g_new() here, catch the failure, free any memory
we've already allocated in the function, and propagate the failure
backwards up the call tree so that it can be handled at the interface
level (probably by saying "requested tile manager too big. Hard luck,
buddy." in a message box).

Anyone have further input on this?

Cheers,
Dave.
Comment 13 Sven Neumann 2003-11-13 21:36:24 UTC
I am repeating myself but this needs to be catched earlier. There is
not much sense in waiting until we run out of memory. Especially since
due to tile swapping we shouldn't run out of memory at all. Anyway, it
will take a looooooong time until the OOM situation occurs.

What needs to be done here is to add a simple confirmation dialog just
like the File->New dialog has. The problem to fix here is not the OOM
situation but the fact that you can easily set an unreasonable scaling
factor.
Comment 14 Dave Neary 2003-11-14 08:10:33 UTC
I disagree for a number of reasons.

First, this is the allocation of a TileManager, nothing to do with
image data (we haven't gotten that far yet). The TileManager needs
references to each of the tiles, and he needs them in memory. This
function is allocating 4096*4096* sizeof(Tile *) bytes + 4096*4096*
sizeof(Tile) bytes - in total that makes at least 900 odd Megabytes of
memory.

Second, the crash is the only part of this bug which is critical.
Adding a pre-scale size check to scale_region() and warning the user
he's requested a huge image is grand, and we should do that too. But
that's a separate issue to the fact that we go OOM and crash. If
someone has 2G of RAM, and 100G of hard disk, and they want to create
260kx260k images, then we should let them.

Third, it's (imho) bad to just let malloc fail and crash the program,
particularly in a part of the code where that might realistically
happen. It's also wrong, in my opinion, to treat the malloc failure at
the place where it's caught (beyond clean-up of the function itself),
since we don't know what operation requested the memory further back
the call tree. We should propagate the failure to the place where it
can be treated at the interface level, as I said before.

In short, this is nothing to do with tile swapping, I think that
preventing the crash is *the* important part of the bug, and while I
agree that warning the user when he might be doing something stupid is
the right thing to do, I think we should also tell him afterwards when
we're sure that he's done something stupid.

Dave.
Comment 15 Dave Neary 2003-11-14 08:17:55 UTC
One question... is there any reason not to replace line 180 above, 
  tiles[k] = g_new (Tile, 1);
with this on line 175 (before the start of the for loops)
  tiles[0] = g_try_malloc (Tile, nrows * ncols);
?

In other words, is there ever a situation where we free one Tile * in
the TileManager without freeing the rest? (I haven't found one)

This would have the additional benefit of being a lot faster, both
freeing and allocating the tile manager.

Cheers,
Dave.
Comment 16 Dave Neary 2003-11-14 08:22:27 UTC
Strike that - tile_invalidate() frees individual tiles and replaces
the pointer with a pointer to an invalid tile. 

Dave.
Comment 17 Sven Neumann 2003-11-14 09:10:31 UTC
The point is that g_try_malloc() wasn't available when this code was
written. If we want to deal with OOM situations that would be a
completely new feature. The current code is not prepared to do this.
So this is certainly not a 2.0 feature since it will probably need a
major overhaul of some very basic GIMP functions. I suggest that we
don't attempt to do this but rather make sure that GEGL doesn't have
the same problem. Let's concentrate on #21028 as a workaround and
postpone this bug until we use GEGL.
Comment 18 Dave Neary 2003-11-14 10:29:27 UTC
As a workaround, then, is there some way that we can do the same thing
as is currently done in file-dialog-new and grey out the "OK" button
completely if the image is bigger than the maximum image size in the
preferences? I'm not very clear on how that's done, except that
somehow the boolean template->initial_size_too_large gets set to true
in gimp_template_notify() in app/core/gimptemplate.c if we go over the
max size limit, and the button gets set insensitive in
file_new_template_notify in app/gui/file-new-dialog.c. 

Could we do something similar and create a temporary template when
scaling, and use the same mechanism? I'm not very clear on how this is
done in the new image dialog.

Dave.
Comment 19 Sven Neumann 2003-11-14 10:34:54 UTC
The fact that the OK button is set insensitive in the File->New Dialog
is wrong and it should be changed (already working on this). The point
is that you can very well create an image larger than 4GB even on a
32bit machine. Please don't copy this part.
Comment 20 Dave Neary 2003-11-14 11:02:36 UTC
Hi,

My problem with *just* addressing 21028 is that while it will warn
someone that they might be doing bad things, we crash if they go ahead
and do them anyway. IMHO, it's better not to crash, and tell them "we
told you so".

If you feel that the OOM part of this bug should not be addressed for
2.0, then, can I take it that you are in favour of moving the
milestone on this bug? I wouldn't be, personally. I think we can
rescue things in tile_manager_get() and keep the change pretty local
(we would need to handle a NULL return from tile_manager_get ()
everywhere it's used though and propagate the failure (eeek!)).

In any case, ideally we would do both (warn and recover). I think
recovering is more important than warning, even though warning is
probably easier.

Cheers,
Dave.
Comment 21 Dave Neary 2003-11-14 22:33:34 UTC
I can confirm that after the latest change to the new image code from Sven (to stop the templates greying out the OK button), this crash also happens in the New Image dialog. Just try to create an image of 256kx256k pixels on a machine with less than a gigabyte of ram + swap, and you will crash from an OOM failure.

Of course, when creating a new image, we get watrned about the enormous memory usage, unlike with a scale.

Comment 22 Sven Neumann 2003-11-14 22:50:38 UTC
Sure but the old limit was completely arbitrary and unreasonable. I
successfully created a 5GB image on an i386 machine today.
Comment 23 Sven Neumann 2003-11-14 22:58:15 UTC
That said, you have to be plain stupid to attempt to create a 256k x
256k image even if the dialog warned you that this image will need
around 500GB...

Let me explain my changes as of today. The reason we had a limit on
the File->New dialog until today was that when this code was written,
we couldn't rely on the availability of 64bit integers. Since glib-2.2
this is guaranteed and glib provides the functions to handle them.
This allowed me to change the memsize related API from gulong to
guint64. Until this change, the memsize related functions and widgets
would have handled values larger than 4GB on 64bit platforms only.
This platform dependency is now gone.
Comment 24 Sven Neumann 2003-11-14 23:09:50 UTC
Also, can we please fix our terminology, the application isn't
crashing. It calls exit() because memory cannot be allocated. Of
course for the user this is the same result (and it should be avoided
if possible), but since we are discussing this between developers
here, we could as well make sure that everyone understands what's
going on.
Comment 25 Dave Neary 2003-11-18 09:10:59 UTC
If we can't agree on a way to fix this, I suggest bumping this crash
to 2.0, and putting in the warning for the scale operation (what you
did in your earlier patch). To my mind the only correct fix is to
defend against malloc failures in this type of situation, but the
warning at least lets people know what they're doing.

Dave.
Comment 26 Sven Neumann 2003-11-18 10:37:33 UTC
Do we disagree about how to fix this? I think we only disagree at to
when to fix this. Changing from 1.3 to 2.0 doesn't change anything
since I don't expect more than two or three weeks between the
prerelease and 2.0. Why don't you just try the g_try_malloc() approach
and if we get somewhere, we can fix it for 2.0, otherwise be bump it
to 2.2.
Comment 27 Dave Neary 2003-11-19 10:49:04 UTC
Agreed. I am currently in the middle of a house move, so I will have
very little time to spend on the GIMP for the next couple of weeks. I
will try to steal some time at work to address this bug, but if I
don't get the time to do it, and no-one else steps forward to have a
go, I agree that once we solve bug #21028, we can bump this to later. 

It is not critical that this be fixed in a development branch only, in
my opinion, a clean fix could (and should) be considered for 2.0.x,
even though we do not expect to have many releases on that branch.

Cheers,
Dave.
Comment 28 Dave Neary 2003-11-20 17:46:12 UTC
Created attachment 21660 [details] [review]
Patch to replace g_new with g_try_malloc in tile_manager_get
Comment 29 Dave Neary 2003-11-20 17:50:14 UTC
I've attached a patch which is very local, and which doesn't work, to
catch a fail in g_try_malloc, and free the resources which have been
allocated in the function up to that point, before returning NULL.

The program dies before passing into the if (tile == NULL) loop, at a
point where the GIMP is taking up all real memory, and most of swap.

I think that it's better to have a warning message in place, and maybe
fix this later, than to have nothing for 2.0.

I will therefore not look at this any more, and will do 21028 instead.
If anyone wants to address this before 2.0, please feel free. I will
leave it on the 1.3.x milestone for the moment, and it will be bumped
when we decide to release a pre-release if it's not fixed at that stage.

Dave.
Comment 30 Dave Neary 2003-12-08 21:56:41 UTC
Changing milestone to 2.2, and lowering severity. Now that 21028 is
fixed, this is not as urgent.

Dave.
Comment 31 Sven Neumann 2004-10-29 22:43:21 UTC
Bumping the milestone for this bug to Future. About time we go finally enter
string freeze.
Comment 32 Michael Schumacher 2005-12-18 15:16:12 UTC
IMO we should have a look at this for 2.4
Comment 33 weskaggs 2006-05-20 22:22:44 UTC
Bumping the milestone to Future again.
Comment 34 Martin Nordholts 2008-03-29 17:13:07 UTC
Setting the 4 year old patch state to 'Obsolete'. It doesn't contribute very much to a proper solution to this issue which would probably be to integrate the GLib GError error reporting system.
Comment 35 Paulo Cosay 2008-08-28 10:20:46 UTC
Hi everyone!

I would like to follow up on this bug. It really "bugs" me.

Though I don't know if some of you recovers from the crash but as for me, I need to hard reset my PC. I'm trying to find the error log for gimp but found none. Can anyone help me? Thanks.

Anyways, as for my situation, it happens during manual scaling with point and click. I think the "slight" problem about rescaling is that whenever we add new layers, we add with the same dimensions as the document. Now, if we just use a small portion of this layer, gimp still treat it as the same dimensions as the document. I think memory consumption can be reduced if we can manage to "shrink" the apportioned layer to the pixel boundaries of the layer, not as the dimensions of the document.
Ex:
Document : 800x600
Layer1: 800x600

I draw a ball in Layer1. After drawing, I think Layer1 should shrink to accommodate only the pixels of the ball. So, either we remove the settings in which we have to input the pixel or dimensions of the new image or let the new layer dimensionless until we place pixels in it.

Also, another situation of mine:

I was just abstract painting layer upon layer. Gimp crashed on my fourth layer when I tried to change my brush's color.

My current settings just before it crashed:
Gimp: 2.4.5
Airbrush: Fuzzy Circle 17, scale 2, rate 80, pressure 20
Image: 800x600 100ppi
Layers: 5 including background, no masks
Mem for undo: 30MB
Mem for tile cache: 1GB
Mem for image: 256MB

Running programs with Gimp:
Xfmedia

My PC:
Xubuntu 8.04
AMD Sempron 2800
512 MB RAM
SIS Mirage
Comment 36 Martin Nordholts 2009-08-26 17:43:30 UTC
*** Bug 592838 has been marked as a duplicate of this bug. ***
Comment 37 Martin Nordholts 2009-08-26 17:47:48 UTC
I think we should not throw a dialog in the face of the user after the user has clicked ok, we can keep the warning within the Scale Dialog itself as soon as the user enters huge numbers. There is no point in waiting with giving the warning.
Comment 38 GNOME Infrastructure Team 2018-05-24 10:42:26 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gimp/issues/23.