Bert Huijben wrote:
> > -----Original Message-----
> > From: Julian Foad [mailto:julianfoad_at_btopenworld.com]
> > Sent: donderdag 18 december 2008 22:54
> > To: svn_at_subversion.tigris.org
> > Subject: svn commit: r34824 - branches/1.5.x
> >
> > Author: julianfoad
> > Date: Thu Dec 18 13:53:47 2008
> > New Revision: 34824
> >
> > Log:
> > * STATUS: Remove my veto and add notes about the resolution.
> >
> > Modified:
> > branches/1.5.x/STATUS
> >
> > Modified: branches/1.5.x/STATUS
> > URL:
> > http://svn.collab.net/viewvc/svn/branches/1.5.x/STATUS?pathrev=34824&r1
> > =34823&r2=34824
> > =======================================================================
> > =======
> > --- branches/1.5.x/STATUS Thu Dec 18 13:04:49 2008 (r34823)
> > +++ branches/1.5.x/STATUS Thu Dec 18 13:53:47 2008 (r34824)
> > @@ -21,6 +21,9 @@ Candidate changes for 1.5.5:
> > (generally missing their first chunk of bytes) which can result
> > in such confusing user scenarios as seeing errors with successful
> > (200) status codes.
> > + Notes:
> > + Simple conflict: use of "SVN_ERR_ASSERT(...)" needs to be
> > replaced
> > + with "#include <assert.h>" and "assert(...)".
>
>
> This replacement is /not/ always safe!
Yes it is. It just depends on your meaning of "safe"!
> E.g. in r34828, which is not proposed for backport:
>
> - if (mb->curr_path)
> - svn_stringbuf_appendbytes(mb->curr_path, cdata, nlen);
> + SVN_ERR_ASSERT(mb->curr_path);
> + svn_stringbuf_appendbytes(mb->curr_path, cdata, nlen);
>
> Would be replaced
>
> By an assert(mb->curr_path);
Correct.
> When this is compiled in release mode the preprocessor removes the assert
> and
>
> * subversion/libsvn_ra_neon/mergeinfo.c
> (cdata_handler): Change bug-hiding "if"s into bug-revealing assertions.
>
> Would become:
>
> * subversion/libsvn_ra_neon/mergeinfo.c
> (cdata_handler): Change bug-hiding in a fatal crash that would use lose
> user code in embedded scenarios (Subclipse, AnkhSVN, etc.)
... if and when such a bug is encountered, which is a small price to pay
compared with the alternative which is silently doing the wrong thing
which might corrupt ("store wrong data in") the user's source code
repository which might not be discovered until months later.
It is always better to reveal a bug sooner than later, even if this
means losing the user's work in progress.
> Note:
> SVN_ERR_ASSERT is /not/ removed by the preprocessor!
> It just returns an error in release mode.
If so configured, yes. I invented it precisely to solve the problem that
you're pointing out. It is not available in v1.5 so we have to live
without it.
> In this specific case this assertion would be a check based on network
> traffic, so this would also introduce a security issue if backported to 1.5
> and a tracable error code in 1.6.
Negative.
Which specific case? You quoted two: r34822 (proposed in r34824) and
r34828. I have examined both.
In both cases, no, they are not checking network data. In both cases, we
are asserting that svn_ra_neon__get_mergeinfo() and its helpers have
correctly initialised their variables.
r34822:
end_element():
SVN_ERR_ASSERT(mb->curr_path->data);
But this pointer is initialised by
svn_ra_neon__get_mergeinfo():
mb.curr_path = svn_stringbuf_create("", pool);
and nothing in the code can make the ->data pointer null thereafter.
r34828:
cdata_handler():
SVN_ERR_ASSERT(mb->curr_path);
Likewise.
Regards,
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=987593
Received on 2008-12-19 17:45:38 CET