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.
