inconsistent null-ptr handling in utf-path related functions

[Stefan <lu...@gmx.de>]

Hi,

looking through the code, I'm wondering that there's some inconsistent 
behavior/handling with regards to calling certain svn_utf-functions with 
a nullptr.

svn_utf__cstring_is_valid() for instance does a nullptr check against 
the provided data and returns FALSE in that case.
However, the callers of this function do not seem to handle that case then:

svn_fs__path_valid(): uses the path then to generate the error output
check_cstring_utf8(): uses it in the strlen()-call
svn_utf_cstring_utf8_width(): accesses the cstr before calling 
svn_utf__cstring_is_valid() so that even if it would be determined to be 
FALSE due to a nullptr, there would have already been an invalid access 
before

I'm not too sure what the API promise is here, but IMO either the 
nullptr-check in svn_utf__cstring_is_valid() is unnecessary or the 
callers are missing the appropriate handling there.

Regards,
Stefan

Re: inconsistent null-ptr handling in utf-path related functions

[Daniel Shahaf <d....@daniel.shahaf.name>]

Stefan Fuhrmann wrote on Tue, Jan 26, 2016 at 09:21:44 +0100:
> From that, I would derive the following heuristics:
> 

Once we agree upon a set of guidelines, it'll be nice to add it to
HACKING, at least as a link to your email.

They heuristics you propose sound good to me, so +0.  (The only reason
I'm not +1 is that I haven't reviewed them carefully enough yet.)

Cheers,

Daniel

> * Path and string validation functions shall not crash upon NULL
>   inputs; error messages for NULL strings should explicitly show
>   "NULL" as the problem.
> 
> * Outside path, error and basic string processing, NULL pointers
>   are invalid for mandatory parameters.  Optional parameters are
>   to be clearly documented as such (e.g. cancellation callbacks).
> 
> * To make high-level functions e.g. within libclient robust against
>   NULL pointers, use assertions.  Don't try to mask those conditions
>   and "limp on".
> 
> Applied to svn_fs__path_valid and friends, they should be fixed
> to exhibit defined behaviour when called with NULL inputs.
> 
> -- Stefan^2.

Re: inconsistent null-ptr handling in utf-path related functions

[Stefan Fuhrmann <st...@apache.org>]

On 25.01.2016 22:30, Stefan wrote:
> Hi,
>
> looking through the code, I'm wondering that there's some inconsistent
> behavior/handling with regards to calling certain svn_utf-functions with a nullptr.
>
> svn_utf__cstring_is_valid() for instance does a nullptr check against the
> provided data and returns FALSE in that case.
> However, the callers of this function do not seem to handle that case then:
>
> svn_fs__path_valid(): uses the path then to generate the error output
> check_cstring_utf8(): uses it in the strlen()-call
> svn_utf_cstring_utf8_width(): accesses the cstr before calling
> svn_utf__cstring_is_valid() so that even if it would be determined to be FALSE
> due to a nullptr, there would have already been an invalid access before
>
> I'm not too sure what the API promise is here, but IMO either the nullptr-check
> in svn_utf__cstring_is_valid() is unnecessary or the callers are missing the
> appropriate handling there.

Here is what I know about our NULL pointer handling:

* Our public API is usually not robust against NULL pointers,
   except where it turned out to be convenient (e.g. some string
   functions). Guideline: If NULL is not mentioned explicitly
   in the docstring, using NULL is not safe.

* At least in the past we supported NULL for root paths (as an
   alternative to "" or "/").  We probably still do today.

* We tend to fail hard and early on unexpected NULL inputs.

* Over time, we added tons of verification code at the "outer"
   server interfaces (RA layer, disk I/O) to prevent bogus data
   from crashing the server.

 From that, I would derive the following heuristics:

* Path and string validation functions shall not crash upon NULL
   inputs; error messages for NULL strings should explicitly show
   "NULL" as the problem.

* Outside path, error and basic string processing, NULL pointers
   are invalid for mandatory parameters.  Optional parameters are
   to be clearly documented as such (e.g. cancellation callbacks).

* To make high-level functions e.g. within libclient robust against
   NULL pointers, use assertions.  Don't try to mask those conditions
   and "limp on".

Applied to svn_fs__path_valid and friends, they should be fixed
to exhibit defined behaviour when called with NULL inputs.

-- Stefan^2.