Proposal for change in standard anti-crash C++ checks

Kent James kent at caspia.com
Wed Nov 6 17:00:25 UTC 2013


On 11/6/2013 12:07 AM, acelists at atlas.sk wrote:
> Hi.
>
> How does the proposal solve the problem when null is a valid return value? We do not want to error out of the caller, we must process it correctly.
The example was meant for the vast majority of cases where we expect a 
ptr to be returned, but we want to check anyway to prevent crashes. So, 
when null is an expected null value, the examples would apply in the 
case where the caller incorrectly assumed that the method was always 
expected to return a value. In that case, with the example, the program 
would return an error. Without the example, the program would crash when 
you returned a null.

When a null is expected or used as a flag, you would not follow the 
example, but instead check for the null and handle it explicitly.

> So in your proposal we would need to really know what are the expected values and do not use NS_ENSURE_STATE if null is valid. So the error handling depends on which method is being used. That seems to me equivalent to using NS_ENSURE_SUCCESS everywhere and then think if null is valid and branch the code later on in the caller (where the returned pointer is used). So I am not sure what improvement this is.
The improvement is that, presently, the assumption is that a valid rv 
implies that the returned pointer is not null, so in lots of code we 
never actually check that the pointer is null before using it. But there 
are existing and proposed cases where that is not true. You may say 
"fine in those cases just check for null ptr and not in others", but who 
knows which routines validly return nulls, and which do not? Plenty of 
existing code has no documentation at all, and even those that do you 
cannot rely on a clear indication of whether a null is valid or not.

If I am going to make a mistake, I want to make a mistake that throws an 
error falsely (in the new proposal) rather than simply crash (in the 
current method). I have done way too many patches to fix crashes that 
consisted of adding null checks when no nulls were expected.  In the 
case of spamsettings, this code here 
http://mxr.mozilla.org/comm-aurora/source/mailnews/base/src/nsMsgDBView.cpp#3576 
with the old assumptions will crash when you modify the expectation of 
null return in spamsettings. You may say "OK then I will fix it", but 
you are assuming that you know that null is valid when rv is OK, but 
many times we don't know, or we make mistakes. Our standards should 
prevent mistakes when we are wrong.

> Also, in your proposal all methods would need to be audited on how they handle the passed in out-argument ("server" in the example). What if you pass non-null in it and the function fails so it should return null.
That is a valid point. My example works when the COMPtr is defined right 
before it (with an implicit null) but that should not be be relied on.

OK then, let's change the proposal to only use the new second macro, 
that is

NS_ENSURE_PTR_SUCCESS(rv, server); as a replacement for NS_ENSURE_SUCCESS(rv, rv).

In the rare case where you want a different rv to be returned, you could 
always use the existing macro.

>   But I think few methods do this today (mostly they error out with rv!=NS_OK without touching the out-argument). I think today we depend on the out-argument value being correct only if rv = NS_OK.
The fact that few modules do  this is why this is important, because 
that makes those few that do have different behavior rare enough that we 
do not bother to look up the behavior for each method before blindly 
just adding NS_ENSURE_SUCCESS(rv, rv);

Also returning a null pointer is much more js-friendly than throwing an 
error, as you can simply have code like:

if (!server.spamsettings) {}

instead of having to wrap it in a try block, so I would like to 
encourage more use of that.




More information about the tb-planning mailing list