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

acelists at atlas.sk acelists at atlas.sk
Wed Nov 6 08:07:00 UTC 2013


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.
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.
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. 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.

I do agree with point 2). If a function uses passed in pointers it should always check them beforehand (NS_ENSURE_ARG_POINTER) regardless what the callers do (they may change). The question is whether to apply this rule to all functions, or only the public ones (NS_IMETHODIMP).

aceman
______________________________________________________________
> Od: Kent James <kent at caspia.com>
> Komu: <tb-planning at mozilla.org>
> Dátum: 06.11.2013 01:18
> Predmet: Proposal for change in standard anti-crash C++ checks
>
>I'd like to have a small discussion about standard ways of checking for 
>errors in C++ code as anti-crash prevention, and propose two code standards.
>
>1) A common pattern in mailnews code is the following:
>
>   nsCOMPtr<nsIMsgIncomingServer> server;
>   nsresult rv = GetServer(getter_AddRefs(server));
>   NS_ENSURE_SUCCESS(rv, rv);
>   return server->GetMsgStore(aStore);
>
>The problem with that is that sometimes we may have code that 
>intentionally returns a null value, as is being proposed in bug 739908 
>for spamSettings. Nobody really knows which code follows which standard, 
>so these checks are always risky.
>
>I would like to propose that, instead, the pattern be the following when 
>all we are doing is preventing crashes, but have every expectation that 
>the getter should succeed:
>
>   nsCOMPtr<nsIMsgIncomingServer> server;
>   nsresult rv = GetServer(getter_AddRefs(server));
>   NS_ENSURE_STATE(server);
>   return server->GetMsgStore(aStore);
>
>This code returns NS_ERROR_UNEXPECTED if the server is null when we are 
>expecting it to be valid.
>
>This has the advantage of 1) it is safer, and 2) it is shorter. It has 
>the disadvantage that the rv is not propagated. So if the error is 
>reasonably
>expected, such that the upstream code might choose to act depending on 
>the rv value, you would not follow this pattern. Instead, you would have 
>to follow:
>
>   nsCOMPtr<nsIMsgIncomingServer> server;
>   nsresult rv = GetServer(getter_AddRefs(server));
>   NS_ENSURE_TRUE(server && NS_SUCCEEDED(rv), NS_FAILED(rv) ? rv : 
>NS_ERROR_FAILURE);
>
>That line is long enough and common enough that it would be worth a 
>standard macro, something like:
>   NS_ENSURE_PTR_SUCCESS(rv, server);
>
>2) As a related standard, all code entering a C++ method has checked 
>pointers. (You do not need to check for null before sending a ptr to a 
>method).
>
>Neil objected to this requirement here: 
>https://bugzilla.mozilla.org/show_bug.cgi?id=522886#c110 with the comment:
>
>"(not needed) because this is an internal method and the callers already 
>null-check their variables before passing them in"
>
>I think that the standard should be that if your method would crash with 
>a null ptr, you must check the value to ensure it is non-null before 
>using it.
>
>Comments?
>
>(And sorry for the early send. Who decided that ctl-Enter sends the 
>message instead of adding a line feed ???)
>_______________________________________________
>tb-planning mailing list
>tb-planning at mozilla.org
>https://mail.mozilla.org/listinfo/tb-planning
>



More information about the tb-planning mailing list