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

[SourceForge.net]

Patches item #1552731, was opened at 2006-09-05 09:47
Message generated for change (Comment added) made by rhettinger
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: Georg Brandl (gbrandl)
Summary: Fix error checks and leaks in setobject.c

Initial Comment:
Check return values for errors.

Fix refcounts on the error returns;

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

>Comment By: Raymond Hettinger (rhettinger)
Date: 2006-09-07 15:13

Message:
Logged In: YES 
user_id=80475

Georg, the buildbot seems to be happy with this patch as
applied to the head, would you please backport it to the
Py2.5 release for me (since I'm away from my svn commit
machine for a while).

Thanks,


Raymond

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

Comment By: Neal Norwitz (nnorwitz)
Date: 2006-09-07 02: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 02: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 02: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-06 01: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