> -----Original Message-----
> From: Branko Čibej [mailto:brane_at_wandisco.com]
> Sent: woensdag 24 april 2013 07:37
> To: dev_at_subversion.apache.org
> Subject: Re: svn commit: r1470936 - in /subversion/trunk/subversion:
> include/svn_io.h libsvn_subr/stream.c libsvn_wc/adm_ops.c
> libsvn_wc/update_editor.c
>
> On 23.04.2013 21:06, Bert Huijben wrote:
> >
> >> -----Original Message-----
> >> From: Ivan Zhakov [mailto:ivan_at_visualsvn.com]
> >> Sent: dinsdag 23 april 2013 20:47
> >> To: Philip Martin
> >> Cc: dev_at_subversion.apache.org
> >> Subject: Re: svn commit: r1470936 - in /subversion/trunk/subversion:
> >> include/svn_io.h libsvn_subr/stream.c libsvn_wc/adm_ops.c
> >> libsvn_wc/update_editor.c
> >>
> >> On Tue, Apr 23, 2013 at 9:09 PM, Philip Martin
> >> <philip.martin_at_wandisco.com> wrote:
> >>> Ivan Zhakov <ivan_at_visualsvn.com> writes:
> >>>
> >>>> On Tue, Apr 23, 2013 at 5:04 PM, <philip_at_apache.org> wrote:
> >>>>> Author: philip
> >>>>> Date: Tue Apr 23 13:04:42 2013
> >>>>> New Revision: 1470936
> >>>>>
> >>>>> URL: http://svn.apache.org/r1470936
> >>>>> Log:
> >>>>> Significantly reduce the number of open files during a typical update
> >>>>> over ra_serf by using lazy-opening streams to delay opening until the
> >>>>> HTTP response is received. This changes the new-in-1.8 lazy-open
> API.
> >>>>>
> >>>> [...]
> >>>>
> >>>>> --- subversion/trunk/subversion/libsvn_wc/update_editor.c (original)
> >>>>> +++ subversion/trunk/subversion/libsvn_wc/update_editor.c Tue
> Apr
> >> 23 13:04:42 2013
> >>>>> @@ -3525,6 +3525,46 @@ open_file(const char *path,
> >>>>> return SVN_NO_ERROR;
> >>>>> }
> >>>>>
> >>>> [...]
> >>>>
> >>>>> + tb = apr_palloc(handler_pool, sizeof(struct lazy_target_baton));
> >>>> apr_pcalloc() ? Otherwise it looks like needless premature
> optimization.
> >>> I prefer alloc with explicit initialisation unless we know zero is a
> >>> correct initialisation. Adding initialisation to zero makes it harder
> >>> to use a tool like valgrind to identify missing initialisations.
> >>>
> >> Ok, but on the other apr_pcalloc() makes code execution stable and
> >> code will crash if not initialized properly instead of accessing
> >> garbage.
> > Maybe we should add our own define for these cases, to have the stable
> behavior of initializing to NULL in released code while still being able to
> disable this for debugging?
>
> That's a /great/ way of making release builds behave differently from
> debug builds.
Well, explicitly not initializing variables is another way of adding undefined behavior.
And I'm not suggesting that we should not enable this for debug builds.
But making things easier to run to valgrind shouldn't be the reason to deliberately deliver less secure code.
The tools should help deliver secure/correct code; not making us rely on undefined behavior in untested code paths.
Bert
Received on 2013-04-24 10:55:15 CEST