[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r26326 - branches/svnpatch-diff/subversion/libsvn_client

From: Peter Hosey <boredzo_at_gmail.com>
Date: 2007-09-10 02:02:31 CEST

On Sep 08, 2007, at 09:14:12, David Glasser wrote:
>> + /* The file 'yours' points to edit_baton's cached
>> empty file. Since
>> + * it was just moved, set it to empty so that later
>> get_empty_file()
>> + * calls don't get it wrong with the cache, i.e.
>> create another
>> + * empty file again. */
>> + sprintf((char *)yours, ""); /* awful cast */
>
> That strikes me as a strange use of sprintf; how about just
> *yours = '\0';
> ?

More importantly, it seems to me that “awful cast” is an understatement.
It ought to be no less than a #warning.

It's not obvious from the diff, since it doesn't cover the function
signature, but “yours” is declared as const char *:

static svn_error_t *
merge_file_added([etc.],
                  const char *mine,
                  const char *older,
----> const char *yours,
                  svn_revnum_t rev1,
                  svn_revnum_t rev2,
                  [etc.])

This is dangerous. It assumes that the storage behind “yours” will never
really be const. This *may* be true now (and I wouldn't know), but even
if it is, can you guarantee that the storage will never be const in
the future?

I apologize if I'm speaking out of turn, given that I'm not an svn
developer, have never submitted a patch for svn, and don't even know the
svn codebase very well. But the don't-write-to-const rule is universal,
so I felt I had to point this out, lest this potential bug become a real
bug at some point in the future.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Sep 10 01:59:21 2007

This is an archived mail posted to the Subversion Dev mailing list.