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