Stefan Sperling wrote:
> On Sun, Jun 08, 2008 at 09:49:35AM +0300, Daniel Shahaf wrote:
>> David Glasser wrote on Sat, 7 Jun 2008 at 23:11 -0700:
>>> Um, I really don't want to drag this process out any longer. But as
>>> far as I can tell, without backporting these revisions, every single
>>> program written against the svn_repos API before 1.5.x (ie, the API
>>> which does not include svn_repos_set_repos_capabilities) which tries
>>> to commit anything to a repository with a start-commit hook will
>>> dereference NULL (ie, crash) every time.
>>>
>>> Please please please somebody convince me that I am wrong in that
>>> conclusion above. Because if we actually claim to care about API
>>> users, that's pretty serious.
>>>
>> The conclusion is correct.
>
> Ooops, I didn't realise that the 1.4.x API was affected.
> Thanks for pointing this out.
>
>> The CLIENT_CAPABILITIES member of svn_repos_t is only written by the
>> new-in-1.5 svn_repos_remember_client_capabilities function, which isn't
>> called by any other svn_repos_* function.
>>
>> However, svn_repos_fs_begin_txn_for_commit2 passes
>> $x=CLIENT_CAPABILITIES to svn_repos__hooks_start_commit(capabilities=$x),
>> which passes it to svn_cstring_join(strings=$x), which dereferences
>> STRINGS in the termination condition of the first for() loop. Since
>> STRINGS ultimately is CLIENT_CAPABILITIES, it is an uninitialised
>> pointer being dereferenced.
>>
>> As Stefan suggested yesterday, another fix could be to initialise
>> client_capabilities to an empty array when creating an svn_repos_t
>> (and change the internal API in repos.h to disallow that array being
>> NULL).
>
> Also, there's another crash bug already waiting for 1.5.1, namely r31223.
> Which is about "Fix segfault when svn_ra_open3 is passed a bogus URL such
> as 'bogusURL'." This affects 1.4 API users as well, because svn_ra_open2
> calls svn_ra_open3, passing the URL parameter through.
>
> r31223 was scheduled for 1.5.1 and not 1.5.0 by Eric because the crash
> cannot happen with our svn(1) client. Because this is true also for the
> (r31620, r31622) group, I opted for scheduling it for 1.5.1 as well.
> Otherwise I would have nominated it for 1.5.0, because yes, crashing APIs
> are really bad.
>
> If we put either fix into 1.5.0, it does not make much sense to exclude
> the other.
>
> I guess Hyrum's not gonna be happy about an -rc10, but I would not object
> putting those fixes into 1.5.0 if more people think it's necessary.
It's not that big of a deal for me personally; I'm just as anxious to
get 1.5.0 out the door as everybody else.
I suggest we move the proposed fixes to the 1.5.0 section of STATUS. We
can continue to debate, and if needed, I'll roll rc10 tomorrow.
-Hyrum
Received on 2008-06-08 18:18:55 CEST