[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

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

From: Stefan Fuhrmann <stefan2_at_apache.org>
Date: Tue, 26 Jan 2016 09:21:44 +0100

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.
Received on 2016-01-26 09:20:58 CET

This is an archived mail posted to the Subversion Dev mailing list.