You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Jelmer Vernooij <je...@samba.org> on 2008/06/22 01:03:44 UTC
[PATCH] Fix unitialized memory access in svn_canonicalize_path()
svn_path_canonicalize() will try to access a single byte before its
allocated buffer if the path specified is "". The attached patch fixes
this. I've confirmed the error and the fix with valgrind.
[[[
* subversion/libsvn_subr/path.c (svn_canonicalize_path): Avoid
accessing unitialized memory when path is "".
]]]
Cheers,
Jelmer
Re: [PATCH] Fix unitialized memory access in svn_canonicalize_path()
Posted by "Daniel L. Rall" <dl...@finemaltcoding.com>.
On Sun, 22 Jun 2008, Jelmer Vernooij wrote:
> Am Samstag, den 21.06.2008, 21:34 -0400 schrieb Karl Fogel:
> > Jelmer Vernooij <je...@samba.org> writes:
> > > svn_path_canonicalize() will try to access a single byte before its
> > > allocated buffer if the path specified is "". The attached patch fixes
> > > this. I've confirmed the error and the fix with valgrind.
> > >
> > > [[[
> > > * subversion/libsvn_subr/path.c (svn_canonicalize_path): Avoid
> > > accessing unitialized memory when path is "".
> > > ]]]
> >
> > Your fix looks correct to me. But I think path=="" is the only case
> > where your dst > canon check would get invoked anyway. If so, a better
> > fix might be to just test for the special case at the top of the
> > function:
> >
> > Index: subversion/libsvn_subr/path.c
> > ===================================================================
> > --- subversion/libsvn_subr/path.c (revision 31834)
> > +++ subversion/libsvn_subr/path.c (working copy)
> > @@ -1248,6 +1248,10 @@
> > apr_size_t canon_segments = 0;
> > svn_boolean_t uri;
> >
> > + /* "" is already canonical */
> > + if (! *path)
> > + return path;
> > +
> > dst = canon = apr_pcalloc(pool, strlen(path) + 1);
> >
> > /* Copy over the URI scheme if present. */
> >
> > Thoughts?
> Yeah, I agree that's a bit clearer way of handling it.
This was committed to trunk in r31837.
Re: [PATCH] Fix unitialized memory access in
svn_canonicalize_path()
Posted by Jelmer Vernooij <je...@samba.org>.
Am Samstag, den 21.06.2008, 21:34 -0400 schrieb Karl Fogel:
> Jelmer Vernooij <je...@samba.org> writes:
> > svn_path_canonicalize() will try to access a single byte before its
> > allocated buffer if the path specified is "". The attached patch fixes
> > this. I've confirmed the error and the fix with valgrind.
> >
> > [[[
> > * subversion/libsvn_subr/path.c (svn_canonicalize_path): Avoid
> > accessing unitialized memory when path is "".
> > ]]]
>
> Your fix looks correct to me. But I think path=="" is the only case
> where your dst > canon check would get invoked anyway. If so, a better
> fix might be to just test for the special case at the top of the
> function:
>
> Index: subversion/libsvn_subr/path.c
> ===================================================================
> --- subversion/libsvn_subr/path.c (revision 31834)
> +++ subversion/libsvn_subr/path.c (working copy)
> @@ -1248,6 +1248,10 @@
> apr_size_t canon_segments = 0;
> svn_boolean_t uri;
>
> + /* "" is already canonical */
> + if (! *path)
> + return path;
> +
> dst = canon = apr_pcalloc(pool, strlen(path) + 1);
>
> /* Copy over the URI scheme if present. */
>
> Thoughts?
Yeah, I agree that's a bit clearer way of handling it.
Cheers,
Jelmer
--
Jelmer Vernooij <je...@samba.org> - http://samba.org/~jelmer/
Jabber: jelmer@jabber.fsfe.org
Re: [PATCH] Fix unitialized memory access in svn_canonicalize_path()
Posted by Karl Fogel <kf...@red-bean.com>.
Jelmer Vernooij <je...@samba.org> writes:
> svn_path_canonicalize() will try to access a single byte before its
> allocated buffer if the path specified is "". The attached patch fixes
> this. I've confirmed the error and the fix with valgrind.
>
> [[[
> * subversion/libsvn_subr/path.c (svn_canonicalize_path): Avoid
> accessing unitialized memory when path is "".
> ]]]
Your fix looks correct to me. But I think path=="" is the only case
where your dst > canon check would get invoked anyway. If so, a better
fix might be to just test for the special case at the top of the
function:
Index: subversion/libsvn_subr/path.c
===================================================================
--- subversion/libsvn_subr/path.c (revision 31834)
+++ subversion/libsvn_subr/path.c (working copy)
@@ -1248,6 +1248,10 @@
apr_size_t canon_segments = 0;
svn_boolean_t uri;
+ /* "" is already canonical */
+ if (! *path)
+ return path;
+
dst = canon = apr_pcalloc(pool, strlen(path) + 1);
/* Copy over the URI scheme if present. */
Thoughts?
-Karl
> === modified file 'subversion/libsvn_subr/path.c'
> --- subversion/libsvn_subr/path.c 2008-06-13 10:43:20 +0000
> +++ subversion/libsvn_subr/path.c 2008-06-22 00:55:30 +0000
> @@ -1310,7 +1310,7 @@
> }
>
> /* Remove the trailing slash if necessary. */
> - if (*(dst - 1) == '/')
> + if (dst > canon && *(dst - 1) == '/')
> {
> /* If we had any path components, we always remove the trailing slash. */
> if (canon_segments > 0)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org