GNOME Bugzilla – Bug 344431
validation of INT8 parameters is broken?
Last modified: 2006-06-26 05:21:08 UTC
Bring up an image, and try the following in the console: drawable = gimp.image_list()[0].layers[0] pdb.plug_in_exchange(drawable.image,drawable, 3, 127, 255,0,0,0,0,0,0) (I remind you that the parameters for plug_in_exchange are: image, drawable, r1, g1, b1, r2, g2, b2, threshold_r, threshold_g, threshold_b) Providing values > 127 for any of the INT8 parameters causes a 'TypeError: invalid arguments'; try replacing '255' with '100' to show that it only does that in those circumstances. .. I think the most correct behaviour here would be to take the least significant byte of the input integers as the INT8 (so, 0..255) and also map -128..-1 to 0x80..0xFF (so that it works right with both signed and unsigned values)
Note: I'm not sure whether this bug is actually in PyGimp or somewhere else. I don't know exactly where this bug originates from. Apparently not plug-in-exchange. PyGimp uses the following line: ret = gimp_run_procedure2(self->name, &nret, self->nparams, params + 1); .. gimp_run_procedure2 is defined in libgimp/gimp.c, which seems to not do any validation. Don't know where to go from there.
The problem is that the validation code assumes a signed 8-bit integer while the plug-in author obviously assumed that INT8 would be an unsigned 8-bit integer.
According to Mitch, my assumption is wrong and the problem seems to be in the Python bindings. We should nevertheless try to figure this out as soon as possbile.
*** Bug 345849 has been marked as a duplicate of this bug. ***
Per bug #345849, this is not python specifiv.
Created attachment 67961 [details] [review] small patch to address the issue This seems to fix it, but perhaps d_int8 should be simply changed to be guint8 to avoid subtle bugs like this elsewhere. Looking at the plug-ins in tree, most of them either think it's unsigned anyway, or tuse it as a boolean.
IMHO we should change it to guint8, esp. because of INT8ARRAY, which appears to be the only way of getting a generic binary blob of data tranfserred over the wire. It makes no sense using signed for that.
Created attachment 67994 [details] [review] Additional patches regarding INT8 being unsigned There seems to be some inconsistency regarding INT8 and whether it is supposed to be a signed or unsigned data type. The d_int8 and d_int8array union members of _GimpParamData defined in libgimp/gimp.h both use gint8 rather than gunit8. In app.pl and pdb.pl found in the tools/pdbgen directory there are indications of INT8 being unsigned but in pdb.pl are calls to g_value_(g|s)et_int() rather than ..._uint().
G_MINUINT8 doesn't exist... did you even test compile the patch?
2006-06-25 Manish Singh <yosh@gimp.org> * app/plug-in/plug-in-params.c * libgimp/gimp.[ch] * libgimpbase/gimpprotocol.[ch] * plug-ins/common/colormap-remap.c * plug-ins/common/curve_bend.c * plug-ins/common/grid.c * plug-ins/pygimp/pygimp-pdb.c * plug-ins/script-fu/siod-wrapper.c * tools/pdbgen/pdb.pl: turn d_int8 and d_int8array into guint8. Fixes bug #344431.