[PATCH] fix osutil.c, error handling, fd leak

[Benoit Boissinot]

On Thu, Sep 11, 2008 at 01:28:01PM -0400, Petr Kodl wrote:
> 
> Isn't this going to decrement Py_None one too many if the PyList_Append
> fails?
> 

You're right, thanks.

updated patch below.

# HG changeset patch
# User Benoit Boissinot <benoit.boissinot at ens-lyon.org>
# Date 1221146411 -7200
# Node ID 31afce165e9bb1ac83f3a35fa22f6ad5116c2093
# Parent  ab57069232b4d9ce95e5898d9154410988008ba2
osutil: proper error handling

Check after every call that might fail, raise the proper exception
(e.g. return NULL after a python function fails).
Fix a filedescriptor that wasn't closed.
The braces lines 164 are on purpose to remove a gcc warning.

diff --git a/mercurial/osutil.c b/mercurial/osutil.c
--- a/mercurial/osutil.c
+++ b/mercurial/osutil.c
@@ -100,7 +100,7 @@
 			   int keep_stat, int *need_stat)
 {
 	struct dirent *ent;
-	PyObject *name, *py_kind, *val;
+	PyObject *name = NULL, *py_kind = NULL, *val = NULL;
 
 #ifdef DT_REG
 	*need_stat = 0;
@@ -130,22 +130,22 @@
 			}
 #endif
 
-		if (kind != -1)
+		if (kind != -1) {
 			py_kind = PyInt_FromLong(kind);
-		else {
+			if (!py_kind)
+				return NULL;
+		} else {
 			py_kind = Py_None;
 			Py_INCREF(Py_None);
 		}
 
+		name = PyString_FromString(ent->d_name);
+		if (!name)
+			goto fail;
+
 		val = PyTuple_New(keep_stat ? 3 : 2);
-		name = PyString_FromString(ent->d_name);
-
-		if (!name || !py_kind || !val) {
-			Py_XDECREF(name);
-			Py_XDECREF(py_kind);
-			Py_XDECREF(val);
-			return PyErr_NoMemory();
-		}
+		if (!val)
+			goto fail;
 
 		PyTuple_SET_ITEM(val, 0, name);
 		PyTuple_SET_ITEM(val, 1, py_kind);
@@ -154,11 +154,18 @@
 			Py_INCREF(Py_None);
 		}
 
-		PyList_Append(list, val);
+		if (PyList_Append(list, val) == -1)
+			goto fail_val;
 		Py_DECREF(val);
 	}
 
-	return 0;
+	return list;
+fail:
+	Py_XDECREF(name);
+	Py_XDECREF(py_kind);
+fail_val:
+	Py_XDECREF(val);
+	return NULL;
 }
 
 static PyObject *statfiles(PyObject *list, PyObject *ctor_args, int keep,
@@ -172,23 +179,28 @@
 	ssize_t size = PyList_Size(list);
 
 	for (i = 0; i < size; i++) {
-		PyObject *elt = PyList_GetItem(list, i);
-		char *name = PyString_AsString(PyTuple_GET_ITEM(elt, 0));
-		PyObject *py_st = NULL;
-		PyObject *py_kind = PyTuple_GET_ITEM(elt, 1);
+		PyObject *elt, *py_kind;
+		char *name;
+			
+		elt = PyList_GetItem(list, i);
+		if (!elt)
+			return NULL;
+		name = PyString_AsString(PyTuple_GET_ITEM(elt, 0));
+		if (!name)
+			return NULL;
+		py_kind = PyTuple_GET_ITEM(elt, 1);
 
-		kind = py_kind == Py_None ? -1 : PyInt_AsLong(py_kind);
-		if (kind != -1 && !keep)
+		if (!keep && py_kind != Py_None)
 			continue;
 
 		strncpy(path + len + 1, name, PATH_MAX - len);
 		path[PATH_MAX] = 0;
 
 		if (keep) {
-			py_st = PyObject_CallObject(
-				(PyObject *)&listdir_stat_type, ctor_args);
+			PyObject *py_st = PyObject_CallObject(
+					(PyObject *)&listdir_stat_type, ctor_args);
 			if (!py_st)
-				return PyErr_NoMemory();
+				return NULL;
 			stp = &((struct listdir_stat *)py_st)->st;
 			PyTuple_SET_ITEM(elt, 2, py_st);
 		}
@@ -202,7 +214,7 @@
 			return PyErr_SetFromErrnoWithFilename(PyExc_OSError,
 							      path);
 
-		if (kind == -1) {
+		if (py_kind == Py_None) {
 			if (S_ISREG(stp->st_mode))
 				kind = S_IFREG;
 			else if (S_ISDIR(stp->st_mode))
@@ -219,18 +231,22 @@
 				kind = S_IFSOCK;
 			else
 				kind = stp->st_mode;
+		} else {
+			kind = PyInt_AsLong(py_kind);
+			if (kind == -1 && PyErr_Occurred())
+				return NULL;
 		}
 
 		if (py_kind == Py_None && kind != -1) {
 			py_kind = PyInt_FromLong(kind);
 			if (!py_kind)
-				return PyErr_NoMemory();
+				return NULL;
 			Py_XDECREF(Py_None);
 			PyTuple_SET_ITEM(elt, 1, py_kind);
 		}
 	}
 
-	return 0;
+	return list;
 }
 
 static PyObject *listdir(PyObject *self, PyObject *args, PyObject *kwargs)
@@ -239,13 +255,13 @@
 	DIR *dir = NULL;
 	PyObject *statobj = NULL;
 	PyObject *list = NULL;
-	PyObject *err = NULL;
+	PyObject *ret = NULL;
 	PyObject *ctor_args = NULL;
 	char *path;
 	char full_path[PATH_MAX + 10];
 	int path_len;
 	int need_stat, keep_stat;
-	int dfd;
+	int dfd = -1;
 
 	if (!PyArg_ParseTupleAndKeywords(args, kwargs, "s#|O:listdir", kwlist,
 					 &path, &path_len, &statobj))
@@ -262,7 +278,7 @@
 	dfd = -1;
 #endif
 	if (!dir) {
-		err = PyErr_SetFromErrnoWithFilename(PyExc_OSError, path);
+		PyErr_SetFromErrnoWithFilename(PyExc_OSError, path);
 		goto bail;
 	}
 
@@ -274,27 +290,31 @@
 	strncpy(full_path, path, PATH_MAX);
 	full_path[path_len] = '/';
 
-	err = listfiles(list, dir, keep_stat, &need_stat);
-	if (err)
+	ret = listfiles(list, dir, keep_stat, &need_stat);
+	if (!ret)
 		goto bail;
 
-	PyList_Sort(list);
+	if (PyList_Sort(list) == -1)
+		goto bail;
 
 	if (!keep_stat && !need_stat)
 		goto done;
 
-	err = statfiles(list, ctor_args, keep_stat, full_path, path_len, dfd);
-	if (!err)
+	ret = statfiles(list, ctor_args, keep_stat, full_path, path_len, dfd);
+	if (ret)
 		goto done;
 
  bail:
+	ret = NULL;
 	Py_XDECREF(list);
 
  done:
 	Py_XDECREF(ctor_args);
 	if (dir)
 		closedir(dir);
-	return err ? err : list;
+	if (dfd != -1)
+		close(dfd);
+	return ret;
 }
 
 

-- 
:wq