[Patch RFC v2]: Ignore symlinks when searching directories

[<bm...@at...>]

Using opcontrol -p can crash if the searched directory contains broken
symlinks. Based on some feedback from Maynard, the existing code
probably intended to ignore symlinks, but didn't work because stat will
never return S_ISLNK on a valid symlink.

This patch converts get_matching_pathnames so that it ignores symlinks
in all cases, regardless of whether the link is to a file, directory or
invalid. It'll issue a warning if lstat fails for some reason.

Tests: this resolves the crash in opreport -p. It also touches the JIT
codepath, which definitely intended to ignore symlinks. JIT is untested.

Steps to reproduce:
$ mkdir /tmp/foo 
$ ln -s /does_not_exist /tmp/foo/bar
$ opreport -p /tmp/foo

Signed-off-by: Brian Bloniarz <bm...@at...>

diff -Naurp -x CVS oprofile.orig/ChangeLog oprofile/ChangeLog
--- oprofile.orig/ChangeLog	2009-05-07 10:20:14.000000000 -0400
+++ oprofile/ChangeLog	2009-05-13 12:26:14.000000000 -0400
@@ -1,3 +1,9 @@
+2009-05-13  Brian Bloniarz  <bm...@at...>
+	* oprofile/libutil/op_file.c:
+	* oprofile/libutil/op_file.h:
+	* oprofile/libutil++/file_manip.h:
+	* oprofile/opjitconv/opjitconv.c: Ignore symlinks when searching directories
+
 2009-05-07  Andi Kleen  <ak...@li...>
 	* utils/ophelp.c:
 	* libop/op_cpu_type.c:
diff -Naurp -x CVS oprofile.orig/libutil/op_file.c oprofile/libutil/op_file.c
--- oprofile.orig/libutil/op_file.c	2008-04-28 17:23:24.000000000 -0400
+++ oprofile/libutil/op_file.c	2009-05-13 11:45:57.000000000 -0400
@@ -94,8 +94,10 @@ static char * make_pathname_from_dirent(
 	name_len = strlen(basedir) + strlen("/") + strlen(ent->d_name) + 1;
 	name = xmalloc(name_len);
 	sprintf(name, "%s/%s", basedir,	ent->d_name);
-	if (stat(name, st_buf) != 0) {
+	if (lstat(name, st_buf) != 0) {
 		free(name);
+		fprintf(stderr, "could not stat directory entry %s/%s\n",
+			basedir, ent->d_name);
 		name = NULL;
 	}
 	return name;
@@ -116,6 +118,8 @@ int get_matching_pathnames(void * name_l
  * filter match, so for simplicity, we perform this match for all recursion
  * types and logically OR the match result with the  value of the passed
  * recursion_type.
+ *
+ * In all cases, symlinks are ignored entirely.
  */
 #define NO_MATCH 0
 #define MATCH 1
@@ -142,18 +146,23 @@ int get_matching_pathnames(void * name_l
 			// nothing to do but continue the loop
 			break;
 		case NO_RECURSION + MATCH:
-			getpathname(ent->d_name, name_list);
+			name = make_pathname_from_dirent(base_dir, ent,
+						       &stat_buffer);
+			if (name && !S_ISLNK(stat_buffer.st_mode))
+				getpathname(ent->d_name, name_list);
+			free(name);
 			break;
 		case MATCH_ANY_ENTRY_RECURSION + MATCH:
 			name = make_pathname_from_dirent(base_dir, ent,
 						       &stat_buffer);
-			if (name && S_ISDIR(stat_buffer.st_mode) &&
-			    !S_ISLNK(stat_buffer.st_mode)) {
-				get_matching_pathnames(
-					name_list, getpathname,
-					name, filter, recursion);
-			} else {
-				getpathname(name, name_list);
+			if (name && !S_ISLNK(stat_buffer.st_mode)) {
+				if (S_ISDIR(stat_buffer.st_mode)) {
+					get_matching_pathnames(
+						name_list, getpathname,
+						name, filter, recursion);
+				} else {
+					getpathname(name, name_list);
+				}
 			}
 			free(name);
 			break;
diff -Naurp -x CVS oprofile.orig/libutil/op_file.h oprofile/libutil/op_file.h
--- oprofile.orig/libutil/op_file.h	2008-04-28 17:23:24.000000000 -0400
+++ oprofile/libutil/op_file.h	2009-05-13 11:29:50.000000000 -0400
@@ -91,7 +91,7 @@ enum recursion_type {
  *
  * Return a list of pathnames under base_dir, filtered by filter and optionally
  * looking in sub-directory. See description above of the recursion_type 
- * parameter for more details.
+ * parameter for more details. Ignores all symlinks.
  *    NOTE: For C clients: Your implementation of the get_pathname_callback
  *          function will probably dynamically allocate storage for elements
  *          added to name_list.  If so, remember to free that memory when it's
diff -Naurp -x CVS oprofile.orig/libutil++/file_manip.h oprofile/libutil++/file_manip.h
--- oprofile.orig/libutil++/file_manip.h	2005-08-10 12:47:17.000000000 -0400
+++ oprofile/libutil++/file_manip.h	2009-05-13 11:27:21.000000000 -0400
@@ -60,7 +60,7 @@ bool op_file_readable(std::string const 
  *
  * create a filelist under base_dir, filtered by filter and optionally
  * looking in sub-directory. If we look in sub-directory only sub-directory
- * which match filter are traversed.
+ * which match filter are traversed. all symlinks will be ignored.
  */
 bool create_file_list(std::list<std::string> & file_list,
 		      std::string const & base_dir,
diff -Naurp -x CVS oprofile.orig/opjitconv/opjitconv.c oprofile/opjitconv/opjitconv.c
--- oprofile.orig/opjitconv/opjitconv.c	2008-06-23 15:46:09.000000000 -0400
+++ oprofile/opjitconv/opjitconv.c	2009-05-13 11:57:34.000000000 -0400
@@ -243,6 +243,8 @@ out:
  * since by agreement, all JIT dump files should be removed every time
  * the user does --reset.  If we do find the matching samples directory,
  * we create an ELF file (<proc_id>.jo) and place it in that directory.
+ * The dumpfile should be a normal file and not a symlink,
+ * get_matching_pathnames will ensure that for us.
  */
 static int process_jit_dumpfile(char const * dmp_pathname,
 				struct list_head * anon_sample_dirs,
@@ -267,21 +269,6 @@ static int process_jit_dumpfile(char con
 	
 	verbprintf(debug, "Processing dumpfile %s\n", dmp_pathname);
 	
-	/* Check if the dump file is a symbolic link.
-	 * We should not trust symbolic links because we only produce normal dump
-	 * files (no links).
-	 */
-	if (lstat(dmp_pathname, &file_stat) == -1) {
-		printf("opjitconv: lstat for dumpfile failed (%s).\n", strerror(errno));
-		rc = OP_JIT_CONV_FAIL;
-		goto out;
-	}
-	if (S_ISLNK(file_stat.st_mode)) {
-		printf("opjitconv: dumpfile path is corrupt (symbolic links not allowed).\n");
-		rc = OP_JIT_CONV_FAIL;
-		goto out;
-	}
-	
 	if (dumpfilename) {
 		size_t tmp_conv_dir_length = strlen(tmp_conv_dir);
 		char const * dot_dump = rindex(++dumpfilename, '.');