[Python-Dev] unintentional and unsafe use of realpath()

[Peter Jones]

[re-adding Python-Dev]

On Wed, 2005-09-14 at 02:00 +0200, Henrik Levkowetz wrote:
> Hi Peter,
> 
> on 2005-09-10 21:29 Peter Jones said the following:
> > Hi,
> > 
> > In Python 2.4.1, Python/sysmodule.c includes a function PySys_SetArgv().
> > One of the things it does is attempt to resolve symbolic links into
> > absolute paths.  Currently, it uses readlink() if configure found that
> > your system supports it, and then it tries to do the same thing again
> > using realpath() if you system supports that.
> > 
> > This seems wrong; there's really no reason to do both.  So here's a
> > patch to move the realpath() usage into a #else following the
> > HAVE_READLINK test:
> > 
> 
> If a path component above the basename is a link, it will not be
> resolved by only doing readlink() on the basename, while realpath() will
> resolve it.  So it seems to me that your proposed patch will accomplish
> less than the current code for systems which have realpath().

What the current code is doing is a buffer overrun.  Nevertheless,
you're right that it would behave differently than the current code if
that code worked as intended.

Here's a different patch; it uses canonicalize_file_name instead, and if
you don't have that it just uses the same codepath that's already in the
tree.  canonicalize_file_name is identical to realpath() when using the
glibc extension of allocating a buffer when you pass in a NULL target,
as in:

fullpath = realpath(path, NULL);

I chose to use canonicalize_file_name simply because the autoconf test
for it is easier than testing to see if the extension works.

Here's the new patch:

--- Python-2.4.1/pyconfig.h.in.canonicalize	2005-09-14 11:47:04.000000000 -0400
+++ Python-2.4.1/pyconfig.h.in	2005-09-14 11:47:02.000000000 -0400
@@ -58,6 +58,9 @@
 /* Define if pthread_sigmask() does not work on your system. */
 #undef HAVE_BROKEN_PTHREAD_SIGMASK
 
+/* Define to 1 if you have the `canonicalize_file_name' function. */
+#undef HAVE_CANONICALIZE_FILE_NAME
+
 /* Define to 1 if you have the `chown' function. */
 #undef HAVE_CHOWN
 
--- Python-2.4.1/Python/sysmodule.c.canonicalize	2005-09-14 11:53:30.000000000 -0400
+++ Python-2.4.1/Python/sysmodule.c	2005-09-14 11:52:04.000000000 -0400
@@ -1184,6 +1184,9 @@
 		char *p = NULL;
 		int n = 0;
 		PyObject *a;
+#ifdef HAVE_CANONICALIZE_FILE_NAME
+                argv0 = canonicalize_file_name(argv0);
+#else /* ! HAVE_CANONICALIZE_FILE_NAME */
 #ifdef HAVE_READLINK
 		char link[MAXPATHLEN+1];
 		char argv0copy[2*MAXPATHLEN+1];
@@ -1256,6 +1259,7 @@
 #endif /* Unix */
 		}
 #endif /* All others */
+#endif /* ! HAVE_CANONICALIZE_FILE_NAME */
 		a = PyString_FromStringAndSize(argv0, n);
 		if (a == NULL)
 			Py_FatalError("no mem for sys.path insertion");
--- Python-2.4.1/configure.in.canonicalize	2005-09-14 11:46:00.000000000 -0400
+++ Python-2.4.1/configure.in	2005-09-14 11:47:22.000000000 -0400
@@ -2096,8 +2096,8 @@
 AC_MSG_RESULT(MACHDEP_OBJS)
 
 # checks for library functions
-AC_CHECK_FUNCS(alarm bind_textdomain_codeset chown clock confstr ctermid \
- execv fork fpathconf ftime ftruncate \
+AC_CHECK_FUNCS(alarm bind_textdomain_codeset canonicalize_file_name chown \
+ clock confstr ctermid execv fork fpathconf ftime ftruncate \
  gai_strerror getgroups getlogin getloadavg getpeername getpgid getpid \
  getpriority getpwent getsid getwd \
  kill killpg lchown lstat mkfifo mknod mktime \

-- 
  Peter