[Patches] [ python-Patches-1552731 ] Fix error checks and leaks in setobject.c

[SourceForge.net]

Patches item #1552731, was opened at 2006-09-05 07:47
Message generated for change (Comment added) made by nnorwitz
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1552731&group_id=5470

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: Core (C code)
Group: Python 2.5
Status: Open
Resolution: Accepted
Priority: 7
Submitted By: Raymond Hettinger (rhettinger)
Assigned to: Raymond Hettinger (rhettinger)
Summary: Fix error checks and leaks in setobject.c

Initial Comment:
Check return values for errors.

Fix refcounts on the error returns;

----------------------------------------------------------------------

>Comment By: Neal Norwitz (nnorwitz)
Date: 2006-09-07 00:05

Message:
Logged In: YES 
user_id=33168

rev 51798 for 2.6 was checked in.

Georg, true. I was thinking that something like this would
be nicer:

	int rv = set_contains_key(so, key);
	if (rv == -1 ||
 	    (rv != 0 && set_add_key(result, key) == -1)) {

I figured the transformation would be easy.  But now that I
look at the code above, I really don't like that at all. 
The only other solution I can think of is to use a goto:

	int rv = set_contains_key(so, key);
	if (rv == -1)
		goto some_error;
	if (rv) {
		if (set_add_key(result, key) == -1) {
some_error:

I'm not really sure I like that either.  So basically no
matter which way the code looks, I'm not gonna be happy. :-)
 Oh well.

Raymond, were you planning on backporting this or did you
want someone else to?  I volunteer to backport to 2.4. ;-)

----------------------------------------------------------------------

Comment By: Georg Brandl (gbrandl)
Date: 2006-09-06 00:09

Message:
Logged In: YES 
user_id=849994

That misses the possibility of set_contains_key's return
value being 0.

----------------------------------------------------------------------

Comment By: Neal Norwitz (nnorwitz)
Date: 2006-09-06 00:04

Message:
Logged In: YES 
user_id=33168

This is fine.  Though I wonder if hunks like this:

-		if (set_contains_key(so, key)) {
+		int rv = set_contains_key(so, key);
+		if (rv == -1) {
+			Py_DECREF(it);
+			Py_DECREF(result);
+			Py_DECREF(key);
+			return NULL;
+		}
+		if (rv) {
 			if (set_add_key(result, key) == -1) {


would be clearer (to me at least) more like:

	if (set_contains_key(so, key) == -1 ||
 	    set_add_key(result, key) == -1) {

That ensures the cleanup is the same.  There are several
similar hunks.

----------------------------------------------------------------------

Comment By: Georg Brandl (gbrandl)
Date: 2006-09-05 23:54

Message:
Logged In: YES 
user_id=849994

Shouldn't this go into 2.5 final?

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1552731&group_id=5470