You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@gmail.com> on 2012/04/19 17:17:19 UTC
Re: svn commit: r1327979 - in /subversion/trunk/subversion:
include/private/svn_repos_private.h libsvn_fs/fs-loader.c libsvn_repos/fs-wrap.c
mod_dav_svn/repos.c
On Apr 19, 2012 11:02 AM, <ph...@apache.org> wrote:
>...
> +++ subversion/trunk/subversion/libsvn_fs/fs-loader.c Thu Apr 19 15:02:17
2012
>...
> + if (kind == svn_node_dir)
> + {
> + svn_error_clear(err);
> + *fs_type = apr_pstrdup(pool, SVN_FS_TYPE_BDB);
> + return SVN_NO_ERROR;
There's no need to dup a constant string.
>...
> + else if (!strcmp(fs_type, "bdb") && !strcmp(ap_show_mpm(),
"event"))
> + serr = svn_error_createf(SVN_ERR_UNSUPPORTED_FEATURE, NULL,
> + "BDB repository at '%s' is not
compatible "
> + "with event MPM",
> + fs_path);
Won't this error appear in the response? In which case, you're leaking a
server path to the client.
>...
Cheers,
-g
Re: svn commit: r1327979 - in /subversion/trunk/subversion:
include/private/svn_repos_private.h libsvn_fs/fs-loader.c
libsvn_repos/fs-wrap.c mod_dav_svn/repos.c
Posted by Daniel Shahaf <da...@elego.de>.
Greg Stein wrote on Thu, Apr 19, 2012 at 11:17:19 -0400:
> Won't this error appear in the response? In which case, you're leaking a
> server path to the client.
No, the code just below the mail's diff context sanitizes the error.
Re: svn commit: r1327979 - in /subversion/trunk/subversion:
include/private/svn_repos_private.h libsvn_fs/fs-loader.c
libsvn_repos/fs-wrap.c mod_dav_svn/repos.c
Posted by Daniel Shahaf <da...@elego.de>.
Greg Stein wrote on Thu, Apr 19, 2012 at 11:17:19 -0400:
> Won't this error appear in the response? In which case, you're leaking a
> server path to the client.
No, the code just below the mail's diff context sanitizes the error.
Re: svn commit: r1327979 - in /subversion/trunk/subversion: include/private/svn_repos_private.h libsvn_fs/fs-loader.c libsvn_repos/fs-wrap.c mod_dav_svn/repos.c
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> I moved that code without considering that. The documentation
> explicitly states that the result is allocated in pool but I think we
> can change that. It also allows me to rename pool to scratch_pool. I
> suppose there is a theoretical problem if libsvn_fs is dynamically
> loaded and the caller is passing a pool with a longer lifetime than the
> library. The constant string in the dynamic library would then have a
> shorter lifetime than it does now. Unlikely in practice I think.
Oops! The other bits of the function still allocate the result so pool
can't be scratch_pool.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: svn commit: r1327979 - in /subversion/trunk/subversion: include/private/svn_repos_private.h libsvn_fs/fs-loader.c libsvn_repos/fs-wrap.c mod_dav_svn/repos.c
Posted by Philip Martin <ph...@wandisco.com>.
Greg Stein <gs...@gmail.com> writes:
> Do we ever unload the modules? I know mod_dav_svn.so can get unloaded. But
> FS modules?
Well, I suppose if mod_dav_svn gets unloaded then libsvn_fs will be
unloaded as well. But anything unloading libsvn_fs is unlikely to want
to access the memory returned by svn_fs_type. It's much like the path
functions in libsvn_subr returning "". It's not an issue.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: svn commit: r1327979 - in /subversion/trunk/subversion:
include/private/svn_repos_private.h libsvn_fs/fs-loader.c libsvn_repos/fs-wrap.c
mod_dav_svn/repos.c
Posted by Greg Stein <gs...@gmail.com>.
On Apr 19, 2012 12:18 PM, "Philip Martin" <ph...@wandisco.com>
wrote:
>
> Greg Stein <gs...@gmail.com> writes:
>
> > On Apr 19, 2012 11:02 AM, <ph...@apache.org> wrote:
> >>...
> >> +++ subversion/trunk/subversion/libsvn_fs/fs-loader.c Thu Apr 19
15:02:17
> > 2012
> >>...
> >> + if (kind == svn_node_dir)
> >> + {
> >> + svn_error_clear(err);
> >> + *fs_type = apr_pstrdup(pool, SVN_FS_TYPE_BDB);
> >> + return SVN_NO_ERROR;
> >
> > There's no need to dup a constant string.
>
> I moved that code without considering that. The documentation
> explicitly states that the result is allocated in pool but I think we
> can change that. It also allows me to rename pool to scratch_pool. I
> suppose there is a theoretical problem if libsvn_fs is dynamically
> loaded and the caller is passing a pool with a longer lifetime than the
> library. The constant string in the dynamic library would then have a
> shorter lifetime than it does now. Unlikely in practice I think.
Do we ever unload the modules? I know mod_dav_svn.so can get unloaded. But
FS modules?
Cheers,
-g
Re: svn commit: r1327979 - in /subversion/trunk/subversion: include/private/svn_repos_private.h libsvn_fs/fs-loader.c libsvn_repos/fs-wrap.c mod_dav_svn/repos.c
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> I moved that code without considering that. The documentation
> explicitly states that the result is allocated in pool but I think we
> can change that. It also allows me to rename pool to scratch_pool. I
> suppose there is a theoretical problem if libsvn_fs is dynamically
> loaded and the caller is passing a pool with a longer lifetime than the
> library. The constant string in the dynamic library would then have a
> shorter lifetime than it does now. Unlikely in practice I think.
Oops! The other bits of the function still allocate the result so pool
can't be scratch_pool.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: svn commit: r1327979 - in /subversion/trunk/subversion: include/private/svn_repos_private.h libsvn_fs/fs-loader.c libsvn_repos/fs-wrap.c mod_dav_svn/repos.c
Posted by Philip Martin <ph...@wandisco.com>.
Greg Stein <gs...@gmail.com> writes:
> On Apr 19, 2012 11:02 AM, <ph...@apache.org> wrote:
>>...
>> +++ subversion/trunk/subversion/libsvn_fs/fs-loader.c Thu Apr 19 15:02:17
> 2012
>>...
>> + if (kind == svn_node_dir)
>> + {
>> + svn_error_clear(err);
>> + *fs_type = apr_pstrdup(pool, SVN_FS_TYPE_BDB);
>> + return SVN_NO_ERROR;
>
> There's no need to dup a constant string.
I moved that code without considering that. The documentation
explicitly states that the result is allocated in pool but I think we
can change that. It also allows me to rename pool to scratch_pool. I
suppose there is a theoretical problem if libsvn_fs is dynamically
loaded and the caller is passing a pool with a longer lifetime than the
library. The constant string in the dynamic library would then have a
shorter lifetime than it does now. Unlikely in practice I think.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: svn commit: r1327979 - in /subversion/trunk/subversion: include/private/svn_repos_private.h libsvn_fs/fs-loader.c libsvn_repos/fs-wrap.c mod_dav_svn/repos.c
Posted by Philip Martin <ph...@wandisco.com>.
Greg Stein <gs...@gmail.com> writes:
> On Apr 19, 2012 11:02 AM, <ph...@apache.org> wrote:
>>...
>> +++ subversion/trunk/subversion/libsvn_fs/fs-loader.c Thu Apr 19 15:02:17
> 2012
>>...
>> + if (kind == svn_node_dir)
>> + {
>> + svn_error_clear(err);
>> + *fs_type = apr_pstrdup(pool, SVN_FS_TYPE_BDB);
>> + return SVN_NO_ERROR;
>
> There's no need to dup a constant string.
I moved that code without considering that. The documentation
explicitly states that the result is allocated in pool but I think we
can change that. It also allows me to rename pool to scratch_pool. I
suppose there is a theoretical problem if libsvn_fs is dynamically
loaded and the caller is passing a pool with a longer lifetime than the
library. The constant string in the dynamic library would then have a
shorter lifetime than it does now. Unlikely in practice I think.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com