You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2003/12/19 02:36:52 UTC
Leaking errors
I am making a patch to fix places where we leak svn_error_t objects, and I have encountered one that I don't know how to handle (and nor, judging by the comment, did the author of the code).
In subversion/mod_dav_svn/update.c (dav_svn__update_report): line 1356:
serr = svn_fs_check_path(&dst_kind, uc.rev_root, dst_path,
resource->pool);
/* ### what to do with this error? */
Some errors in that function are handled like this (which is 15 lines above it):
serr = svn_repos_finish_report(rbaton);
if (serr)
return dav_svn_convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
"A failure occurred while "
"driving the update report editor");
How should failure of svn_fs_check_path be handled here?
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Leaking errors
Posted by Erik Huelsmann <e....@gmx.net>.
> On Thu, 2003-12-18 at 20:36, Julian Foad wrote:
>
> > How should failure of svn_fs_check_path be handled here?
>
> I don't think there's anything wrong with wrapping the serr in a
> dav_svn_convert_err(). You've got the right idea.
Isn't there a possibility we start leaking internal paths to the outside
world when doing that?
bye,
Erik.
--
+++ GMX - die erste Adresse für Mail, Message, More +++
Neu: Preissenkung für MMS und FreeMMS! http://www.gmx.net
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Leaking errors
Posted by Julian Foad <ju...@btopenworld.com>.
Philip Martin wrote:
> Ben Collins-Sussman <su...@collab.net> writes:
>
>>On Thu, 2003-12-18 at 20:36, Julian Foad wrote:
>>
>>>How should failure of svn_fs_check_path be handled here?
>>
>>I don't think there's anything wrong with wrapping the serr in a
>>dav_svn_convert_err(). You've got the right idea.
>
> Does dav_svn_convert_err do the right thing? It accepts an
> svn_error_t* and returns a dav_error*, but it doesn't appear to clear
> the svn_error_t pool. So it looks like every dav_svn_convert_err
> leaks. Fixing it could be ugly; simply adding an svn_error_clear
> won't work since the dav error itself is allocated in the svn_error_t
> pool. I think dav_svn_convert_err may need to grow a pool parameter.
Well spotted. I'll try to look at it next month. Unfortunately I'll be pretty much off-line for a couple of weeks from tomorrow.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Leaking errors
Posted by Philip Martin <ph...@codematters.co.uk>.
Ben Collins-Sussman <su...@collab.net> writes:
> On Thu, 2003-12-18 at 20:36, Julian Foad wrote:
>
>> How should failure of svn_fs_check_path be handled here?
>
> I don't think there's anything wrong with wrapping the serr in a
> dav_svn_convert_err(). You've got the right idea.
Does dav_svn_convert_err do the right thing? It accepts an
svn_error_t* and returns a dav_error*, but it doesn't appear to clear
the svn_error_t pool. So it looks like every dav_svn_convert_err
leaks. Fixing it could be ugly; simply adding an svn_error_clear
won't work since the dav error itself is allocated in the svn_error_t
pool. I think dav_svn_convert_err may need to grow a pool parameter.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: PATCH: Leaking errors
Posted by Erik Huelsmann <e....@gmx.net>.
> kfogel@collab.net wrote:
> > Julian Foad <ju...@btopenworld.com> writes:
> >
> >>Committed in r8049.
> >
> > Remember, if you think it should be a 1.0 candidate, file it with
> > "r8049" in the summary somewhere.
>
> Sure. I don't think it should.
Too bad you're offline already. I was going to ask you if you found it 1.0.1
material or that you want to push this into the next minor release number
(ie. 1.1 - if that's what it will be)? You seem to have fixed quite a lot of
memory leaks with your patch.
bye,
Erik.
--
+++ GMX - die erste Adresse für Mail, Message, More +++
Neu: Preissenkung für MMS und FreeMMS! http://www.gmx.net
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: PATCH: Leaking errors
Posted by Julian Foad <ju...@btopenworld.com>.
kfogel@collab.net wrote:
> Julian Foad <ju...@btopenworld.com> writes:
>
>>Committed in r8049.
>
> Remember, if you think it should be a 1.0 candidate, file it with
> "r8049" in the summary somewhere.
Sure. I don't think it should.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: PATCH: Leaking errors
Posted by kf...@collab.net.
Julian Foad <ju...@btopenworld.com> writes:
> Committed in r8049.
Remember, if you think it should be a 1.0 candidate, file it with
"r8049" in the summary somewhere.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: PATCH: Leaking errors
Posted by Julian Foad <ju...@btopenworld.com>.
Ben Collins-Sussman wrote:
> On Fri, 2003-12-19 at 11:50, Julian Foad wrote:
>
>>OK, thanks. With that in place, here's a patch to fix some more error leaks that I found by looking through the code.
>>
>>Should I commit this?
>
> Wow, nice cleanup. Heck, if it passes 'make check' on trunk, commit it
> there. Backporting to 1.0 is a separate question. :-)
Committed in r8049.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: PATCH: Leaking errors
Posted by Ben Collins-Sussman <su...@collab.net>.
On Fri, 2003-12-19 at 11:50, Julian Foad wrote:
> OK, thanks. With that in place, here's a patch to fix some more error leaks that I found by looking through the code.
>
> Should I commit this?
Wow, nice cleanup. Heck, if it passes 'make check' on trunk, commit it
there. Backporting to 1.0 is a separate question. :-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
PATCH: Leaking errors
Posted by Julian Foad <ju...@btopenworld.com>.
Ben Collins-Sussman wrote:
> On Thu, 2003-12-18 at 20:36, Julian Foad wrote:
>
>>How should failure of svn_fs_check_path be handled here?
>
> I don't think there's anything wrong with wrapping the serr in a
> dav_svn_convert_err(). You've got the right idea.
OK, thanks. With that in place, here's a patch to fix some more error leaks that I found by looking through the code.
Should I commit this?
- Julian
Re: Leaking errors
Posted by Ben Collins-Sussman <su...@collab.net>.
On Thu, 2003-12-18 at 20:36, Julian Foad wrote:
> How should failure of svn_fs_check_path be handled here?
I don't think there's anything wrong with wrapping the serr in a
dav_svn_convert_err(). You've got the right idea.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org