On 04/18/2012 12:15 AM, Julian Foad wrote:
Blair Zajac wrote:
In case of an illegal svn_checksum_kind_t being passed to
svn_checksum__from_digest(), I want to change it from
svn_checksum_t *
svn_checksum__from_digest(const unsigned char *digest,
svn_checksum_kind_t kind,
apr_pool_t *result_pool);
to
svn_error_t *
svn_checksum__from_digest(svn_checksum_t **checksum,
const unsigned char *digest,
svn_checksum_kind_t kind,
apr_pool_t *result_pool);
Wouldn't a better API be:
svn_checksum_t *
svn_checksum__from_digest_md5(digest, result_pool);
svn_checksum_t *
svn_checksum__from_digest_sha1(digest, result_pool);
since all current callers want just one specific kind (apart from internal
calls from the implementations of svn_checksum_dup and
svn_checksum_empty_checksum)?
Notice that svn_checksum_empty_checksum() returns NULL if the kind is not
recognized, while svn_checksum_dup() does no such check and would pass the
unknown kind into ...from_digest().
Therefore it appears that _dup() is the only caller that could have been
passing a bad kind -- at least in current trunk code; it may be different in
whatever version you're running. And so, to fix the crash, you will still need
to decide how _dup() should handle a bogus checksum struct. You could have
_dup() return NULL, but unless the callers check for null that would just defer
the crash. It would be a little better to call
SVN_ERR_MALFUNCTION_NO_RETURN(), to give the client program a little more
awareness of the abnormal termination.
That's what I'm seeing, _dup() is calling __from_digest(). It appears
to be a lifetime issue where it's trying to dup a svn_checksum_t *
pointing to bad data.
It would be nice if _dup() returned a svn_error_t * instead of causing a
core dump later or calling SVN_ERR_MALFUNCTION_NO_RETURN(), but that
would be a messier API and not consistent with all our other _dup()'s.
Too bad we're not working in C++ where we could throw an exception.
This is to prevent a core dump we've observed with our RPC server.
The function is in a public .h file but shown as private. Do I need to add
svn_checksum__from_digest2() or can I just change it?
We're allowed to just change it, as I understand it.
(The closest thing I can find in 'hacking'
is<http://subversion.apache.org/docs/community-guide/conventions.html#interface-visibility>.
But that doesn't seem to mention the nuances of ABI vs. API and the issue of, IIRC,
all symbols in a shared library getting into a Windows DLL ABI even if they're
supposed to be private, or whatever exactly that issue was.)
Thanks. I'm just deleting it following this discussion.
Blair
