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 155733 - need to check return values of gimp_drawable_mask_bounds()
need to check return values of gimp_drawable_mask_bounds()
Status: RESOLVED FIXED
Product: GIMP
Classification: Other
Component: Plugins
git master
Other All
: High normal
: 2.10
Assigned To: GIMP Bugs
GIMP Bugs
Depends on:
Blocks:
 
 
Reported: 2004-10-18 14:56 UTC by Michael Natterer
Modified: 2016-04-18 20:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch for the empty selection bug in a number of plug-in (12.04 KB, patch)
2008-07-28 17:02 UTC, Luidnel Maignan
needs-work Details | Review
Patch for the empty selection bug in a number of plug-in (12.07 KB, patch)
2008-07-28 21:31 UTC, Luidnel Maignan
committed Details | Review
patch gimpressionist plugin bugzilla #155733 (3.26 KB, patch)
2010-09-06 14:05 UTC, lloyd konneker
needs-work Details | Review
Revised patch to gimpressionist (3.04 KB, patch)
2010-09-08 12:35 UTC, lloyd konneker
committed Details | Review
Fix empty selection segfault. (620 bytes, patch)
2011-04-09 22:46 UTC, Brennan Shacklett
committed Details | Review
Improves empty selection handling in several plugins (23.71 KB, patch)
2011-04-11 02:05 UTC, Brennan Shacklett
committed Details | Review
Fixes for edge-sobel, noise-randomize, maze, pagecurl and fractal explorer (13.95 KB, patch)
2012-02-17 18:19 UTC, Ville Sokk
committed Details | Review
Attempt to remove gimp_drawable_mask_bounds() in cml-explorer.c (4.26 KB, patch)
2015-08-29 11:17 UTC, Andrew Worsley
none Details | Review
Removed gimp_drawable_mask_bounds() from border-average.c (2.39 KB, patch)
2015-08-30 01:27 UTC, Andrew Worsley
committed Details | Review
Mask intersect fix for photocopy plug-in. (1001 bytes, patch)
2015-09-15 20:03 UTC, Louis Benazet
none Details | Review
Second attempt at fix for photocopy plugin (984 bytes, patch)
2015-09-15 20:33 UTC, Louis Benazet
committed Details | Review
Removed gimp_drawable_mask_bounds() from color-cube (1.90 KB, patch)
2015-10-02 01:54 UTC, Andrew Worsley
committed Details | Review
Fix two bugs in fractal-trace - needed before removing gimp_drawable_mask_bounds() (1.87 KB, patch)
2015-10-02 11:21 UTC, Andrew Worsley
committed Details | Review
Removed gimp_drawable_mask_bounds() from fractal-trace curve-bend emboss (3.20 KB, patch)
2015-10-02 11:24 UTC, Andrew Worsley
none Details | Review
Removed gimp_drawable_mask_bounds() from edge-neon plugin (3.62 KB, patch)
2015-10-03 03:42 UTC, Andrew Worsley
none Details | Review
Remove gimp_drawable_mask_bounds from plug-ins (9.91 KB, patch)
2015-10-04 11:11 UTC, Andrew Worsley
none Details | Review
Remove gimp_drawable_mask_bounds from plug-ins (20.47 KB, patch)
2015-10-14 10:30 UTC, Andrew Worsley
committed Details | Review
Removes calls to gimp_drawable_mask_bounds() rest of plugins except python script plugin (28.33 KB, patch)
2015-10-28 11:37 UTC, Andrew Worsley
committed Details | Review
Add missing menu item for plug-ins to allow testing removal of gimp_drawable_mask_bounds() works (2.22 KB, patch)
2015-10-28 11:42 UTC, Andrew Worsley
committed Details | Review
Remove last gimp_drawable_mask_bounds() call in the plugins (1.90 KB, patch)
2015-10-28 11:46 UTC, Andrew Worsley
committed Details | Review
fix missing semicolon on in emboss (752 bytes, patch)
2016-04-18 09:23 UTC, Andrew Worsley
committed Details | Review

Description Michael Natterer 2004-10-18 14:56:09 UTC
An easy way to break lots of GIMP functions and plug-ins:

- Have a layer smaller than the image.
- Have a selection which does not intersect with the layer.
- do something.

Result: GIMP warns like crazy, messes its undo stacks,
        or just completely crashes out.

The problem is that gimp_image_mask_bounds() returns TRUE,
but an empty area (which is absolutely correct).

To fix this, all callers of gimp_image_mask_bounds() need to
check the returned area for validity.

I suggest to add a "gboolean *area_non_empty" or "area_valid" return
value to the core API, so it will compile only after all places
have been looked at. For plug-ins, we have to live with the manual
return value checking for the time being...
Comment 1 Michael Natterer 2004-10-18 14:57:02 UTC
Oops, s/gimp_image_mask_bounds/gimp_drawable_mask_bounds/g of course.
Comment 2 Simon Budig 2004-10-18 15:10:27 UTC
I suggest to introduce gimp_image_mask_bounds which returns the bounds in image
coordinates and changing the return value of gimp_drawable_mask_bounds to return
a boolean indicating if there is a selection AND the selection intersects with
the drawable.
Comment 3 Michael Natterer 2004-10-18 15:43:28 UTC
1. There already is gimp_selection_bounds().
2. Changing the meaning of a return value is no option because
   it would break plug-ins.
Comment 4 Michael Natterer 2004-10-19 22:52:53 UTC
Fixed in CVS for the core:

2004-10-20  Michael Natterer  <mitch@gimp.org>

	* app/core/gimpdrawable.[ch]: added gimp_drawable_mask_intersect()
	which returns the same bounding box as gimp_drawable_mask_bounds(),
	but returns TRUE only if there is a non-empty intersection between
	the drawable and the selection, or no selection at all. It also
	returns the intersection as x,y,width,height instead of the
	eeky x1,y1,x2,y2.

	* app/core/gimp-edit.c
	* app/core/gimpdrawable-blend.c
	* app/core/gimpdrawable-bucket-fill.c
	* app/core/gimpdrawable-desaturate.c
	* app/core/gimpdrawable-equalize.c
	* app/core/gimpdrawable-histogram.c
	* app/core/gimpdrawable-invert.c
	* app/core/gimpdrawable-stroke.c
	* app/core/gimpimagemap.c
	* app/core/gimpselection.c
	* tools/pdbgen/pdb/color.pdb
	* tools/pdbgen/pdb/transform_tools.pdb: either switch from
	gimp_drawable_mask_bounds() to _intersect() or check the return
	values of _mask_bounds() manually to avoid operations on empty
	areas. Return successfully because it's a nop, not a failure.
	Fixes bug #155733 for the core.

	* app/pdb/color_cmds.c
	* app/pdb/transform_tools_cmds.c: regenerated.
Comment 5 Michael Natterer 2004-10-19 23:32:51 UTC
Fixed in libgimp:

2004-10-20  Michael Natterer  <mitch@gimp.org>

	Fixed bug #155733 for libgimp:

	* tools/pdbgen/pdb/drawable.pdb: export drawable_mask_intersect()
	to the PDB and improved documentation for drawable_mask_bounds().

	* app/pdb/drawable_cmds.c
	* app/pdb/internal_procs.c
	* libgimp/gimpdrawable_pdb.[ch]: regenerated.

	* libgimp/gimp.def: changed accordingly.
Comment 6 weskaggs 2005-03-21 23:59:53 UTC
Some of the plug-ins:

2005-03-21  Bill Skaggs  <weskaggs@primate.ucdavis.edu>

	* plug-ins/common/sparkle.c
	* plug-ins/common/spheredesigner.c
	* plug-ins/common/struc.c
	* plug-ins/common/tileit.c
	* plug-ins/common/warp.c
	* plug-ins/common/whirlpinch.c
	* plug-ins/common/wind.c: handle situation where intersection
	of selection and drawable is empty; progress on bug #155733.
Comment 7 weskaggs 2006-06-16 16:28:42 UTC
Upgrading target from Future to 2.6, because if anything more needs to be done we should do it, and otherwise close the bug report.
Comment 8 Martin Nordholts 2008-05-25 16:03:56 UTC
I think we can safely assume that there are no severe errors left since almost 2 years has passed now. Severe errors would have been discovered already.
Comment 9 Michael Natterer 2008-05-28 22:47:05 UTC
Did you try that at all? Almost every plug-in is affected, just
try a simple blur.

Should definitely be on the 2.8 milestone so it jumps at people's
eyes so they maybe fix one or two plugins.
Comment 10 Luidnel Maignan 2008-07-28 17:02:29 UTC
Created attachment 115437 [details] [review]
Patch for the empty selection bug in a number of plug-in

I try to join the effort with this patch that add the *_mask_intersect support along with the message telling "Region affected by plug-in is empty" in a number of plug-in. That message comes from one of the plug-in, and I though it was a good solution.

As a first step to a common management of that bug, I propose to use g_message to signal the emptyness of the selection when the plug-in don't have any dialog, and to use a message dialog when the plug-in have a dialog. The aim is to respect the feedback expected by the user. I didn't respect this spec in my patch yet.

(First submission, tell me if any mistake in the protocol)
Comment 11 Sven Neumann 2008-07-28 18:30:55 UTC
Thanks for your help with this. There are some coding style issues with your patch. There should always be a space before an opening parenthesis:

  if(! gimp_drawable_mask_intersect (drawable->drawable_id, ...

should be

  if (! gimp_drawable_mask_intersect (drawable->drawable_id, ...


  g_message(_("Region ...

should be

  g_message (_("Region ...

Please correct that and attach an updated patch.
Comment 12 Luidnel Maignan 2008-07-28 21:31:08 UTC
Created attachment 115455 [details] [review]
Patch for the empty selection bug in a number of plug-in

Adjusting the parenthesis to the coding style
Comment 13 Luidnel Maignan 2008-07-28 22:17:28 UTC
Comment on attachment 115455 [details] [review]
Patch for the empty selection bug in a number of plug-in

In blinds.c:
+                                      &sel_x1, &sel_y1, &sel_width, &sel_height))

instead of

+                                      &sel_x1, &sel_y1, &sel_width, &sel_width))
Comment 14 Michael Natterer 2009-06-07 21:55:23 UTC
It was about time to commit this patch:

commit 9b6c9e1fe4f46d2d47c6d97d4147cf060abd07f8
Author: Michael Natterer <mitch@gimp.org>
Date:   Sun Jun 7 23:52:37 2009 +0200

    Bug 155733 – need to check return values of gimp_drawable_mask_bounds()
    
    Finally commit the patch from Luidnel Maignan, but don't spit messages
    when the effected region is empty (core functions don't spit messages
    either). Also got rid of some x2 and y2 variables that are not needed
    any longer.
Comment 15 Michael Natterer 2009-06-07 22:00:07 UTC
Setting gnome-love because this is definitely a low hanging fruit.
Comment 16 Martin Nordholts 2009-07-16 09:00:32 UTC
Just clarifying what needs to be done here:

Search the code base for occurrences of gimp_drawable_mask_bounds() with

  cd ~/source/gimp
  git grep gimp_drawable_mask_bounds

and replace that with

  if (! gimp_drawable_mask_intersect (drawable->drawable_id,
                                      &x1, &y1, &width, &height))
    return;

in manner similar to Luidnel's patch:

  git show 9b6c9e1fe4f46d2d47c6d97d4147cf060abd07f8

Raising priority to High as it would be very nice to have this fixed for 2.8.
Comment 17 Martin Nordholts 2010-06-22 17:14:54 UTC
We have to prioritize, and there are more important things for us to focus on for 2.8. Moving to Future.
Comment 18 lloyd konneker 2010-09-06 14:05:01 UTC
Created attachment 169577 [details] [review]
patch gimpressionist plugin bugzilla #155733
Comment 19 lloyd konneker 2010-09-06 14:08:20 UTC
Only one plugin patched in my attachment.  If the patch is accepted, I hope to make a larger patch for other plugins.

The patch is as suggested above, except a possible problem with the patch: it gives a warning to the status bar.  See discussion in the patch message.

I have a Python script that tests most plugins for this bug.  It could also test other cases (given image file input that demonstrates a bug.)  It also regression tests (compares current output to prior, reference output images.)  I would be glad to share it.
Comment 20 Sven Neumann 2010-09-06 19:15:02 UTC
Review of attachment 169577 [details] [review]:

Please do not use g_message() for this. If you want a plug-in to return an error message, then the plug-in should fail with status GIMP_PDB_EXECUTION_ERROR and return the error message as second return parameter of type GIMP_PDB_STRING. See plug-ins/common/web-browser.c as an example.
Comment 21 lloyd konneker 2010-09-08 12:35:26 UTC
Created attachment 169761 [details] [review]
Revised patch to gimpressionist
Comment 22 Sven Neumann 2010-09-08 19:49:45 UTC
Review of attachment 169761 [details] [review]:

Thanks. Pushed with some minor coding style fixes.
Comment 23 Brennan Shacklett 2011-04-09 22:46:04 UTC
Created attachment 185619 [details] [review]
Fix empty selection segfault.

This is my first patch, so it only contains a single plugin fix.
If I succeeded on correctly formatting and coding, I intend to fix more of the plugins, so please let me know if I have a mistake. I based my change off of the patch by Luidnel Maignan.
Comment 24 Michael Natterer 2011-04-10 11:13:18 UTC
Comment on attachment 185619 [details] [review]
Fix empty selection segfault.

Thanks, renamed x1,y1 to x,y and pushed
Comment 25 Brennan Shacklett 2011-04-11 02:05:58 UTC
Created attachment 185677 [details] [review]
Improves empty selection handling in several plugins

Most of the changes are trivial. When possible, I removed x2 variables and changed the x1's to x. Same with the y's. Depth merge required more changes, because I had to change the DepthMerge_construct function so it would return a gboolean because it needs to indicate to the run function that it should exit. Hopefully that is an acceptable fix. The patch also disables gimpressionist's error message when dealing with an empty selection.
Comment 26 Michael Natterer 2011-05-06 23:14:05 UTC
Comment on attachment 185677 [details] [review]
Improves empty selection handling in several plugins

Thanks, committed and fixed some things. Next time please make sure the patch actually works :) Didn't push the fix to gimppressionist because that plugin is interactive-only and should say something if nothing is done.
Comment 27 Kevin Cozens 2012-02-11 13:44:50 UTC
A quick search of the plug-ins directory looking for gimp_drawable_mask_bounds() found 58 references across 49 files.

The function call was found in the following files:
gfig/gfig.c, maze/maze.c, color-rotate/color-rotate-utils.c,
pagecurl/pagecurl.c, gimpressionist/brush.c,
fractal-explorer/fractal-explorer.c, gradient-flare/gradient-flare.c, lighting/lighting-image.c, pygimp/pygimp-drawable.c,
map-object/map-object-image.c

In plug-ins/common directory:
edge-sobel.c, tile-seamless.c, color-cube-analyze.c, nova.c, nl-filter.c, sinus.c, unsharp-mask.c, lens-flare.c, fractal-trace.c, noise-randomize.c, smooth-palette.c, engrave.c, illusion.c, softglow.c, sphere-designer.c, convolution-matrix.c, hot.c, cml-explorer.c, lens-apply.c, edge-laplace.c, newsprint.c, grid.c, tile-glass.c, qbist.c, shift.c, sample-colorize.c, cubism.c, curve-bend.c, edge-dog.c, van-gogh-lic.c, blur-motion.c, oilify.c, edge-neon.c, tile-paper.c, sparkle.c, edge.c, border-average.c, photocopy.c, emboss.c
Comment 28 Ville Sokk 2012-02-12 18:53:23 UTC
Do all occurrences of gimp_drawable_mask_bounds() need to be replaced? Not just the ones that break something?
Comment 29 Michael Natterer 2012-02-12 20:43:43 UTC
Just the ones where applying to a zero region freaks out the plugin.
Comment 30 Ville Sokk 2012-02-17 18:19:56 UTC
Created attachment 207878 [details] [review]
Fixes for edge-sobel, noise-randomize, maze, pagecurl and fractal explorer

I changed the following plug-ins: edge-sobel, noise-randomize, maze, pagecurl and fractal explorer. Only pagecurl was actually annoying - it produced a lot of error dialogs. All the others didn't actually affect the user in my opinion - edge-sobel, noise-randomize and maze segfaulted and fractal-explorer gave some kind of error to the terminal that I can't recall. There were of course many plug-ins that gave errors but didn't cause problems. I didn't test lighting-image, pygimp-drawable and map-object-image. Also, maze still uses gimp_drawable_mask_bounds because of its different semantics (that plug-in works differently if you have a selection or not) but doesn't segfault anymore.
Comment 31 Michael Natterer 2012-03-04 16:54:54 UTC
Thanks, pushed to master:

commit 2084b1336955082add85d708135690b12419e899
Author: Ville Sokk <embassyhill@gmail.com>
Date:   Fri Feb 17 19:56:11 2012 +0200

    Bug 155733 - need to check return values of gimp_drawable_mask_bounds()
    
    Mask intersect fixes for some plug-ins.

 plug-ins/common/edge-sobel.c                 |   29 ++++++++---------
 plug-ins/common/noise-randomize.c            |   42 ++++++++++++-------------
 plug-ins/fractal-explorer/fractal-explorer.c |   15 ++++-----
 plug-ins/maze/maze.c                         |   18 ++++++-----
 plug-ins/maze/maze.h                         |    4 +-
 plug-ins/pagecurl/pagecurl.c                 |   32 ++++++++-----------
 6 files changed, 66 insertions(+), 74 deletions(-)
Comment 32 AbdealiJK 2015-08-12 16:45:57 UTC
Im wondering why this big is still open, if the patch is pushed to master. Can I do something to resolve it if its not yet resolved ?
Comment 33 Michael Natterer 2015-08-20 22:57:45 UTC
git grep gimp_drawable_mask_bounds plug-ins

Until every of these places checks the return value, or better
uses gimp_drawable_mask_intersect(), this bug is no fixed.

gimp_drawable_mask_bounds() should probably be deprecated in
favor of gimp_drawable_mask_intersect().
Comment 34 Andrew Worsley 2015-08-29 11:17:07 UTC
Created attachment 310244 [details] [review]
Attempt to remove gimp_drawable_mask_bounds() in cml-explorer.c

As this is my first proposed patch for gimp/gnome I am not sure what the function CML_main_function() does and hence the correctness of returning GIMP_PDB_EXECUTION_ERROR if gimp_drawable_mask_intersect() fails.

Otherwise I think it's okay.

If you think this is ok - I am happy to proceed to remove gimp_drawable_mask_bounds() from more functions.
Comment 35 Andrew Worsley 2015-08-30 01:27:43 UTC
Created attachment 310299 [details] [review]
Removed gimp_drawable_mask_bounds() from border-average.c

Probably mostly needs to check I am handling the case gimp_drawable_mask_intersect() returns failure properly.
Comment 36 Louis Benazet 2015-09-15 20:03:34 UTC
Created attachment 311409 [details] [review]
Mask intersect fix for photocopy plug-in.
Comment 37 Louis Benazet 2015-09-15 20:33:42 UTC
Created attachment 311410 [details] [review]
Second attempt at fix for photocopy plugin

My previous patch was wrong, sorry. This one looks more correct.
Comment 38 Andrew Worsley 2015-10-02 01:54:25 UTC
Created attachment 312538 [details] [review]
Removed gimp_drawable_mask_bounds() from color-cube

Also adds a menu entry for Color-cube so I could debug it.
Took me ages to figure out how to run it.
Has lots of deprecated gegl related function calls - but fixing that should be
another patch/bug.
Comment 39 Michael Natterer 2015-10-02 06:16:13 UTC
I should finally get to the patches here :)
Comment 40 Andrew Worsley 2015-10-02 11:21:54 UTC
Created attachment 312558 [details] [review]
Fix two bugs in fractal-trace - needed before removing gimp_drawable_mask_bounds()

Please apply the bug fixes to fractal-trace plugin first
then the following patch to remove gimp_drawable_mask_bounds()

I thought it better to separate the fixes into a separate patch in case they wanted to be applied separately - pity I can't upload two patches at once
Comment 41 Andrew Worsley 2015-10-02 11:24:01 UTC
Created attachment 312559 [details] [review]
Removed gimp_drawable_mask_bounds() from fractal-trace curve-bend emboss

Some more gimp_drawable_mask_bounds() removals - including the fixed version of fractal-trace
Comment 42 Andrew Worsley 2015-10-03 03:42:46 UTC
Created attachment 312595 [details] [review]
Removed gimp_drawable_mask_bounds() from edge-neon plugin

Remove gimp_drawable_mask_bounds() from edge-neon plugin

Also avoid null pointer crash by returning if called with null drawable
which appears to happen once on the initial update. Instead prints a message
for some one who might want to fix it.
Comment 43 Andrew Worsley 2015-10-04 11:11:10 UTC
Created attachment 312625 [details] [review]
Remove gimp_drawable_mask_bounds from plug-ins

Remove gimp_drawable_mask_bounds plug-ins:
grid hot newsprint nl-filter oilify qbist sample-colorize softglow and
 smooth-palette which also had it's menu entry added for testing
   * Revert "plugins: remove smooth-palette from menus"
    commit 7e6b27609e02220f1c10b9ac3f82c3064a1f57d6.
Comment 44 Michael Natterer 2015-10-10 15:09:02 UTC
Comment on attachment 310299 [details] [review]
Removed gimp_drawable_mask_bounds() from border-average.c

Pushed, minus the change to the dialog function, the selection can
change while th dialog is open.

commit a3f2b1ab6854f3c1afa24b9d32a993678c3d9228
Author: Andrew Worsley <amworsley@gmail.com>
Date:   Sun Aug 30 11:22:27 2015 +1000

    Bug 155733 - need to check return values of gimp_drawable_mask_bounds()
    
    Remove gimp_drawable_mask_bounds() from border-average.c

 plug-ins/common/border-average.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)
Comment 45 Michael Natterer 2015-10-10 15:14:09 UTC
Comment on attachment 311410 [details] [review]
Second attempt at fix for photocopy plugin

Pushed, plus some variable cleanup:

commit e7715dca354eb1b76813065c3483fd50bfb62153
Author: Louis Benazet <louisbenaze@gmail.com>
Date:   Tue Sep 15 20:33:51 2015 +0200

    Bug 155733 - need to check return values of gimp_drawable_mask_bounds()
    
    Mask intersect fix for photocopy plug-in.

 plug-ins/common/photocopy.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)
Comment 46 Michael Natterer 2015-10-10 15:18:19 UTC
Comment on attachment 312538 [details] [review]
Removed gimp_drawable_mask_bounds() from color-cube

Pushed, plus coding style cleanup. Please try to honor the coding style.

commit 24d844c9f1936e480cedd744fc9d624a3408b82e
Author: Andrew Worsley <amworsley@gmail.com>
Date:   Fri Oct 2 11:45:37 2015 +1000

    Bug 155733 - need to check return values of gimp_drawable_mask_bounds()
    
    Remove gimp_drawable_mask_bounds() from color-cube-analyze.c, also add
    menu entry to access it.

 plug-ins/common/color-cube-analyze.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)
Comment 47 Michael Natterer 2015-10-10 19:46:18 UTC
Comment on attachment 312558 [details] [review]
Fix two bugs in fractal-trace - needed before removing gimp_drawable_mask_bounds()

Pushed to master:

commit cc9c7aa3b39383bb1bcc1442e1b07a2ca698ff78
Author: Andrew Worsley <amworsley@gmail.com>
Date:   Fri Oct 2 20:45:21 2015 +1000

    plug-ins: fix 2 bugs in fractal-trace
    
    - crashes when computing % 0 when selection height < 100
    - used y2 instead of x2 when computing width
    - crash when selection in lower left corner

 plug-ins/common/fractal-trace.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
Comment 48 Michael Natterer 2015-10-10 19:52:57 UTC
Andrew, your last 3 patches don't return if intersect() returns FALSE,
but that's the purpose of this entire bug :) Am I missing something?
Comment 49 Andrew Worsley 2015-10-11 05:46:12 UTC
I thought about it but I don't know how to test that case and I am not sure what to do about it so I figured if this was important some one would put me right. See my much earlier patch in comment#34. If you can clarify these points I would be happy to re-do these patches (or add another if that's preferred) after making and testing the changes.

Should a plug-in set status=GIMP_PDB_EXECUTION_ERROR or what ever and return?
How can a create a selection that doesn't intersect the image and cause gimp_drawable_mask_intersect() to return FALSE ?
Comment 50 Michael Natterer 2015-10-11 11:26:42 UTC
Make a new image, move the layer around a bit and make a selection on the
empty background, done :)

In such a case, you would *successfully* do nothing, as in simply
return SUCCESS.
Comment 51 Andrew Worsley 2015-10-14 10:30:37 UTC
Created attachment 313254 [details] [review]
Remove gimp_drawable_mask_bounds from plug-ins

Remove gimp_drawable_mask_bounds() from 14 plug-ins with gimp_drawable_mask_intersect() and checks the return value. This replaces some previous patches as it also does the checks now.

I did test the plug-ins for an empty selection as per comment#50. There are still warnings and error calls they appear to be from the code that calls the plug-ins so beyond my understanding and probably scope of this bug.

e.g.
 GIMP Warning
Unable to cut or copy because the selected region is empty.

  GIMP Error
Execution error for 'Curve Bend':
Cannot operate on empty selections.
(softglow:10374): LibGimpWidgets-CRITICAL **: gimp_preview_set_bounds: assertion 'xmax > xmin' failed
(edge-neon:10375): LibGimpWidgets-CRITICAL **: gimp_preview_set_bounds: assertion 'xmax > xmin' failed

Filter/Distort/curve_bend
(curve-bend:10377): GLib-WARNING **: (/tmp/buildd/glib2.0-2.44.1/./glib/gerror.c:383):g_error_new_valist: runtime check failed: (domain != 0)

Filters/Distort/emboss
(emboss:13585): LibGimpWidgets-CRITICAL **: gimp_preview_set_bounds: assertion 'xmax > xmin' failed
(emboss:13585): LibGimp-CRITICAL **: gimp_pixel_rgn_get_rect: assertion 'y >= 0 && y + height <= pr->drawable->height' failed
Comment 52 Andrew Worsley 2015-10-28 11:37:11 UTC
Created attachment 314307 [details] [review]
Removes calls to gimp_drawable_mask_bounds() rest of plugins except python script plugin

Along with the previous patch 313254 this removes the gimp_drawable_mask_bounds() and replaces it with gimp_drawable_mask_intersect() from the rest of the plugins (except the python script plugin which is addressed in another patch).

These plug-ins now check for the return value and have been tested with the case where there is *NO* intersection. They do not crash but sometimes they report warnings and errors in the logs. This may be code outside the plugin.
Comment 53 Andrew Worsley 2015-10-28 11:42:19 UTC
Created attachment 314311 [details] [review]
Add missing menu item for plug-ins to allow testing removal of gimp_drawable_mask_bounds() works

In order to test some plugins which where patched to fixed this bug they needed menu items to be added back in. This patch made separately to the fix so it can be easily skipped over if you want to keep the plugins out of the menu entry.

I have to say I can not see the point of patching and maintaining plug-in code if it is not accessible to the gimp user.
Comment 54 Andrew Worsley 2015-10-28 11:46:35 UTC
Created attachment 314312 [details] [review]
Remove last gimp_drawable_mask_bounds() call in the plugins

This eliminates the last call in the plugins. It is in a separate patch as this last one is the python plugin patch. Not only was the call to gimp_drawable_mask_bounds() replaced with a call to gimp_drawable_mask_intersect() but a new method was added to drawable mask_intersect() which returns the same result as gimp_drawable_mask_intersect() or an exception if there is no overlap.

 After this and previous patches are applied this bug can be now closed
Comment 55 Michael Natterer 2015-10-28 11:49:12 UTC
Thanks Andrew, will look into applying the patches soon.
Comment 56 Michael Natterer 2016-04-17 20:48:18 UTC
Comment on attachment 313254 [details] [review]
Remove gimp_drawable_mask_bounds from plug-ins

Pushed to master with some cleanup:

commit 4f99c7294a8ea279549e5b5a2e267ce64b88005e
Author: Andrew Worsley <amworsley@gmail.com>
Date:   Sat Aug 29 21:04:09 2015 +1000

    Bug 155733 - need to check return values of gimp_drawable_mask_bounds()
    
    Remove gimp_drawable_mask_bounds() from several plug-ins
    
    Also avoid null pointer crash by returning if called with null
    drawable which appears to happen once on the initial update. Instead
    prints a message for some one who might want to fix it
    
    Change gimp_drawable_mask_intersect() to check return value
    
    We still get some GIMP Error and Warnings but no crashes now

 plug-ins/common/cml-explorer.c    | 31 +++++++++++++++++--------------
 plug-ins/common/curve-bend.c      | 15 ++++++++++++---
 plug-ins/common/edge-neon.c       | 24 +++++++++++-------------
 plug-ins/common/emboss.c          | 10 ++++++----
 plug-ins/common/fractal-trace.c   | 20 +++++++++++++++-----
 plug-ins/common/grid.c            | 12 +++++++++---
 plug-ins/common/hot.c             |  9 +++++----
 plug-ins/common/newsprint.c       |  9 ++++++++-
 plug-ins/common/nl-filter.c       | 11 ++++++-----
 plug-ins/common/oilify.c          |  9 ++++++---
 plug-ins/common/qbist.c           | 27 +++++++++++++++++----------
 plug-ins/common/sample-colorize.c |  9 +++++++--
 plug-ins/common/smooth-palette.c  | 11 ++++++-----
 plug-ins/common/softglow.c        | 15 +++++----------
 14 files changed, 130 insertions(+), 82 deletions(-)
Comment 57 Massimo 2016-04-18 06:16:30 UTC
(In reply to Michael Natterer from comment #56)
> Comment on attachment 313254 [details] [review] [review]
> Remove gimp_drawable_mask_bounds from plug-ins
> 
> Pushed to master with some cleanup:
> 
> commit 4f99c7294a8ea279549e5b5a2e267ce64b88005e
> Author: Andrew Worsley <amworsley@gmail.com>
> Date:   Sat Aug 29 21:04:09 2015 +1000
> 
>     Bug 155733 - need to check return values of gimp_drawable_mask_bounds()
>     
>     Remove gimp_drawable_mask_bounds() from several plug-ins
>     
>     Also avoid null pointer crash by returning if called with null
>     drawable which appears to happen once on the initial update. Instead
>     prints a message for some one who might want to fix it
>     
>     Change gimp_drawable_mask_intersect() to check return value
>     
>     We still get some GIMP Error and Warnings but no crashes now
> 
>  plug-ins/common/cml-explorer.c    | 31 +++++++++++++++++--------------
>  plug-ins/common/curve-bend.c      | 15 ++++++++++++---
>  plug-ins/common/edge-neon.c       | 24 +++++++++++-------------
>  plug-ins/common/emboss.c          | 10 ++++++----
>  plug-ins/common/fractal-trace.c   | 20 +++++++++++++++-----
>  plug-ins/common/grid.c            | 12 +++++++++---
>  plug-ins/common/hot.c             |  9 +++++----
>  plug-ins/common/newsprint.c       |  9 ++++++++-
>  plug-ins/common/nl-filter.c       | 11 ++++++-----
>  plug-ins/common/oilify.c          |  9 ++++++---
>  plug-ins/common/qbist.c           | 27 +++++++++++++++++----------
>  plug-ins/common/sample-colorize.c |  9 +++++++--
>  plug-ins/common/smooth-palette.c  | 11 ++++++-----
>  plug-ins/common/softglow.c        | 15 +++++----------
>  14 files changed, 130 insertions(+), 82 deletions(-)

After this commit gimp master does not build with clang:

emboss.c:358:2: error: void function 'emboss' should not return a value [-Wreturn-type]
 return
 ^
1 error generated.
Makefile:3650: set di istruzioni per l'obiettivo "emboss.o" non riuscito
make[3]: *** [emboss.o] Errore 1
Makefile:651: set di istruzioni per l'obiettivo "all-recursive" non riuscito
make[2]: *** [all-recursive] Errore 1
Makefile:787: set di istruzioni per l'obiettivo "all-recursive" non riuscito
make[1]: *** [all-recursive] Errore 1
Makefile:688: set di istruzioni per l'obiettivo "all" non riuscito
make: *** [all] Errore 2

Here:

https://git.gnome.org/browse/gimp/tree/plug-ins/common/emboss.c#n358

the return without ';' is interpreted as return x1 = ;
Comment 58 Andrew Worsley 2016-04-18 09:23:09 UTC
Created attachment 326229 [details] [review]
fix missing semicolon on in emboss

I'm impressed by clang's error checking - gcc didn't see it.
Comment 59 Michael Natterer 2016-04-18 11:45:58 UTC
Comment on attachment 326229 [details] [review]
fix missing semicolon on in emboss

Thanks, pushed to master:

commit d7efab3daffcc10b98f9d123e17ed3ed1f04db2e
Author: Andrew Worsley <amworsley@gmail.com>
Date:   Mon Apr 18 19:20:39 2016 +1000

    fix missing semicolon in emboss

 plug-ins/common/emboss.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Comment 60 Michael Natterer 2016-04-18 20:04:04 UTC
Comment on attachment 314307 [details] [review]
Removes calls to gimp_drawable_mask_bounds() rest of plugins except python script plugin

Pushed to master:

commit cfa9132c4db8c29a3eb76a1b1ab1ddbc86a8065a
Author: Andrew Worsley <amworsley@gmail.com>
Date:   Mon Oct 26 21:54:39 2015 +1100

    Bug 155733 - need to check return values of gimp_drawable_mask_bounds()
    
    Remove calls to gimp_drawable_mask_bounds() from most plug-ins.
    This just leaves a python gimp interface plug-in.

 plug-ins/common/edge-dog.c                   | 46 ++++++++++++++++++++--------------------------
 plug-ins/common/sparkle.c                    | 28 ++++++++++++++++++----------
 plug-ins/common/sphere-designer.c            | 34 +++++++++++++++++-----------------
 plug-ins/common/unsharp-mask.c               | 11 ++++++-----
 plug-ins/common/van-gogh-lic.c               | 25 ++++++++++++-------------
 plug-ins/fractal-explorer/fractal-explorer.c | 27 ++++++++++++++-------------
 plug-ins/gfig/gfig.c                         | 14 ++++++++------
 plug-ins/gimpressionist/brush.c              | 24 ++++++++++++++----------
 plug-ins/gradient-flare/gradient-flare.c     | 26 +++++++++++++-------------
 plug-ins/lighting/lighting-image.c           | 13 +++++++++++--
 plug-ins/lighting/lighting-main.c            |  8 ++++----
 plug-ins/lighting/lighting-ui.c              |  5 ++---
 plug-ins/map-object/map-object-image.c       |  9 +++++----
 plug-ins/map-object/map-object-main.c        |  8 ++++----
 plug-ins/map-object/map-object-preview.c     |  3 +++
 15 files changed, 151 insertions(+), 130 deletions(-)
Comment 61 Michael Natterer 2016-04-18 20:07:27 UTC
Comment on attachment 314311 [details] [review]
Add missing menu item for plug-ins to allow testing removal of gimp_drawable_mask_bounds() works

Pushed to master:

commit 455a91819bf3ec7b5c73055b57db0b55bc04bd65
Author: Andrew Worsley <amworsley@gmail.com>
Date:   Wed Oct 28 21:38:55 2015 +1100

    plug-ins: add back some menu entries for plugins
    
    So we see what we have. Need some other way of dealing with this stuff...

 plug-ins/common/sphere-designer.c     | 2 ++
 plug-ins/common/unsharp-mask.c        | 2 ++
 plug-ins/gimpressionist/gimp.c        | 2 ++
 plug-ins/map-object/map-object-main.c | 2 ++
 4 files changed, 8 insertions(+)
Comment 62 Michael Natterer 2016-04-18 20:13:58 UTC
Comment on attachment 314312 [details] [review]
Remove last gimp_drawable_mask_bounds() call in the plugins

And the last one:

commit 69e5c553d45ff3bfb1148ad7253417b588bf307e
Author: Andrew Worsley <amworsley@gmail.com>
Date:   Wed Oct 28 22:02:01 2015 +1100

    Bug 155733 - need to check return values of gimp_drawable_mask_bounds()
    
    Add mask_intersect() to pygimp that allows access to
    gimp_drawable_mask_intersect() and returns an exception if there is no
    overlap.

 plug-ins/pygimp/pygimp-drawable.c | 11 +++++++++++
 1 file changed, 11 insertions(+)