[Bf-committers] Re: [Bf-blender-cvs] CVS commit: blender/source/blender/python/api2_2x Armature.c

[Yann Vernier]

On Sat, Dec 24, 2005 at 11:43:35PM +0100, Alexander Ewering wrote:
> 
> On Sat, 24 Dec 2005, Gilbert, Joseph T. wrote:
> 
> >So the bug you found was not fixed by your commit? Please add this to the 
> >bug tracker as well so we can follow your work outside of the mailing list 
> >and also that we may be assured that we can fix this.
> >thx
> 
> The commit was just for optimization and fixes *one* of the two buffer
> overflows, as the new code is more optimal and only needs *one* buffer -
> which can still be overflowed, though.
> 
> It doesn't really make sense to put this in the bug tracker, as the whole
> python API is full of such flaws.

I'm not sure it's quite the whole thing, though it's much of it. From
what I recall the armature code is the worst part of it. It's also rife
with gotos as someone believes in exiting a function from the end; the
technique with gotos I've shown before is useful because *cleanup* code
can be written once, but in these cases that's jarringly absent. 

For instance gen_utils.c has code that tries to use "" as a buffer in
EXPP_{obj,int}Error, but those specific functions are only called from
Armature.c and Bone.c. 

It's fairly consistent that the error paths are nonsensical. Many of
them crash and when they do return an exception it's likely to be
misleading. Worse than that, some of the normal cases have confusing
effects:
 * Trying to fetch things by name might return None, where ordinary
   dictionaries would raise KeyError.
 * Setting some booleans is apparently required to be done using True or
   False. One case of this was reported as a crash bug; the reason is
   the intError bug I mention above. This is inconsistent with Python's
   idea of Trueness, which has a trivial interface.
 * Worse, *reading* booleans is apparently meant to return 1 or 0
   rather than True or False (gen_utils.c). Having a variable refuse
   to accept the value it contains is unacceptable. 

I'm not sure we have that third case, as the case I checked now (in
Armature.c naturally) returns True or False. Many of the gen_utils
functions seem superfluous to me. Consider:

EXPP_incr3(o1,o2,o3);

vs:

Py_INCREF(o1);
Py_INCREF(o2);
Py_INCREF(o3);

Is the latter really that much harder to write? More importantly, the
latter shows the sequence in which the changes happen. For DECREF, that
may be important. 

Likewhise, the functions EXPP_{int,obj}Error() are fairly complex code
compared to what they replace:

return EXPP_intError(PyExc_ValueError, "%s%s", s1, s2);

vs:

PyErr_Format(PyExc_ValueError, "%s%s", s1, s2);
return -1;

In addition to having to manage varargs, there are two functions for the
huge difference of returning NULL or -1. I'm attaching a patch that
should make those two functions work. This fixes the crash, not that the
exception raised is of the wrong type. Testing it I also found that
Blender.Armature.New() had disappeared - while unnecessary, what
warrants such a disruptive API change? If that's fair game I don't see
why I can't rip into the worse oddities, like the odd exception types. 
Clarification desired.

-- 
PGP fingerprint = 9242 DC15 2502 FEAB E15F  84C6 D538 EC09 5380 5746
-------------- next part --------------
Index: gen_utils.c
===================================================================
RCS file: /cvsroot/bf-blender/blender/source/blender/python/api2_2x/gen_utils.c,v
retrieving revision 1.40
diff -u -p -r1.40 gen_utils.c
--- gen_utils.c	10 Dec 2005 19:36:05 -0000	1.40
+++ gen_utils.c	25 Dec 2005 11:21:13 -0000
@@ -176,27 +176,27 @@ int EXPP_ReturnIntError( PyObject * type
 
 int EXPP_intError(PyObject *type, const char *format, ...)
 {
-	char *error = "";
+	PyObject *error;
 	va_list vlist;
 
 	va_start(vlist, format);
-	vsprintf(error, format, vlist);
+	error = PyString_FromFormatV(format, vlist);
 	va_end(vlist);
 
-	PyErr_SetString(type, error);
+	PyErr_SetObject(type, error);
 	return -1;
 }
 //Like EXPP_ReturnPyObjError but takes a printf format string and multiple arguments
 PyObject *EXPP_objError(PyObject *type, const char *format, ...)
 {
-	char *error = "";
+	PyObject *error;
 	va_list vlist;
 
 	va_start(vlist, format);
-	vsprintf(error, format, vlist);
+	error = PyString_FromFormatV(format, vlist);
 	va_end(vlist);
 
-	PyErr_SetString(type, error);
+	PyErr_SetObject(type, error);
 	return NULL;
 }