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
Received on 2012-04-19 00:17:02 CEST