Re: Changing svn_checksum__from_digest()'s signature

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.

> 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.)

- Julian
Received on 2012-04-18 09:16:11 CEST