Please use "error and out" logic (Re: [PATCH] Avoid privileged segfault in GetNCChanges and add many more tests)

[Andrew Bartlett]

On Wed, 2017-08-09 at 09:14 +0200, Stefan Metzmacher wrote:
> Hi Andrew,
> 
> > -	sid = pytalloc_get_ptr(py_sid);
> > +	if (PyObject_TypeCheck(py_sid, &dom_sid_Type)) {
> > +		sid = pytalloc_get_ptr(py_sid);
> > +	} else {
> > +		PyErr_SetString(PyExc_TypeError,
> > +				"expected security.dom_sid "
> > +				"for second argument to .from_sddl");
> > +		return NULL;
> > +
> > +	}
> >  
> >  	secdesc = sddl_decode(NULL, sddl, sid);
> >  	if (secdesc == NULL) {
> > -- 
> 
> Can we please agree on using an "error and out logic" for new code?
> There's really no good reason to have the additional indentation
> for the "sid = pytalloc_get_ptr(py_sid);" line!
> I thought this was part of README.Coding for a long time already,
> but I can't find it...

Sure.  I'm happy to fix this up.

> The above example seems harmless, but I notice new examples
> almost every time I review patches from you or once you reviewed.
> These act as bad examples others will likely copy'n'paste or
> at least learn from.
> 
> This morning I also noticed
> 
> +	if (ret == LDB_SUCCESS) {
> +		if (result->count == 0) {
> +			ret = LDB_ERR_NO_SUCH_OBJECT;
> +		} else {
> +			struct ldb_message *match =
> +				get_best_match(dn, result);
> +			if (match == NULL) {
> +				TALLOC_FREE(frame);
> +				return LDB_ERR_OPERATIONS_ERROR;
> +			}
> +			*msg = talloc_move(mem_ctx, &match);
> +		}
> +	}
> => here
> +	TALLOC_FREE(frame);
> +	return ret;
> 
> Which adds a lot of useless indentation and it makes it really
> hard to add new code where I added "here", as you need remember
> and understand all the logic be had above or protect
> the new code by using another "if (ret == LDB_SUCCESS) {"
> 
> It should be:
> 
> 	if (ret != LDB_SUCCESS) {
> 		TALLOC_FREE(frame);
> 		return ret;
> 	}
> 
> 	if (result->count == 0) {
> 		TALLOC_FREE(frame);
> 		return LDB_ERR_NO_SUCH_OBJECT;
> 	}
> 
> 	match = get_best_match(dn, result);
> 	if (match == NULL) {
> 		TALLOC_FREE(frame);
> 		return LDB_ERR_OPERATIONS_ERROR;
> 	}
> 
> 	*msg = talloc_move(mem_ctx, &match);
> 	TALLOC_FREE(frame);
> 	return LDB_SUCCESS; <= This is important to be not "return ret;"

I'll work with Gary to fix this one up.

> Untangling these kind of spaghetti was the major pain I had when
> working on spnego.c, it took days in order to understand the logic
> of some functions.
> 
> I know we have to live with our existing code for a long time,
> but we should really try hard to avoid it for new code.
> 
> I started to write such a mail already a few weeks ago,
> but discarded it in order to enjoy my vacation...
> 
> (I hope this mail is not too harsh, but reviewing this of stuff
> ruins my day)

Do you have any thoughts on the substantive part of the patch, being
the segfault fix and the tests?

Andrew Bartlett

-- 
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba