GNOME Bugzilla – Bug 78064
Entering large dimensions in Scale Image causes fatal error
Last modified: 2018-05-24 10:42:26 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.
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 ?!
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.
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.
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.
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)?
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.
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.
Is there a quick fix for this that could be put in place without adding a new option? Dave.
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?
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.
yes, just aborting is bad. Very bad. Data loss bad. :)
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):
+ Trace 41731
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.
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.
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.
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.
Strike that - tile_invalidate() frees individual tiles and replaces the pointer with a pointer to an invalid tile. Dave.
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.
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.
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.
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.
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.
Sure but the old limit was completely arbitrary and unreasonable. I successfully created a 5GB image on an i386 machine today.
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.
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.
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.
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.
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.
Created attachment 21660 [details] [review] Patch to replace g_new with g_try_malloc in tile_manager_get
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.
Changing milestone to 2.2, and lowering severity. Now that 21028 is fixed, this is not as urgent. Dave.
Bumping the milestone for this bug to Future. About time we go finally enter string freeze.
IMO we should have a look at this for 2.4
Bumping the milestone to Future again.
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.
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
*** Bug 592838 has been marked as a duplicate of this bug. ***
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.
-- 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.