bin/144411: [patch] mtree(8) doesn't reject non-regular files for -X

[Garrett Cooper]

On Sun, Aug 29, 2010 at 3:55 AM, Garrett Cooper <gcooper at freebsd.org> wrote:
> On Sun, Aug 29, 2010 at 3:54 AM, Garrett Cooper <gcooper at freebsd.org> wrote:
>> On Tue, Mar 30, 2010 at 5:40 PM, Garrett Cooper <gcooper at freebsd.org> wrote:
>>> On Tue, Mar 30, 2010 at 12:12 PM, Bruce Evans <brde at optusnet.com.au> wrote:
>>>> On Wed, 31 Mar 2010, Bruce Evans wrote:
>>>>
>>>>> On Tue, 30 Mar 2010, Garrett Cooper wrote:
>>>>>
>>>>>> Hi,
>>>>>>    I'm not 100% satisfied with this patch now. Looking back it fails
>>>>>> the following case:
>>>>>>
>>>>>>     -P    Do not follow symbolic links in the file hierarchy, instead
>>>>>> con-
>>>>>>           sider the symbolic link itself in any comparisons.  This is the
>>>>>>           default.
>>>>>
>>>>> -P should have the same semantics and description in all utilities.  The
>>>>> description should not have grammar errors like the above (comma splice).
>>>>> ...
>>>>> I now see that the grammar error is from the original version of mtree(1),
>>>>> and is probably one of the things you don't like.  mtree also has -L, but
>>>>> not -R or -P or -h.  It is not clear how any utility that traverses trees
>>>>> can work without a full complement of -[HLPR] or how any utility that
>>>>> ...
>>>>
>>>> Looking at the actual patch, I now see that it is about a completely
>>>> different problem.  You would only need to understand the amount of
>>>> brokenness of -P to see if you need to use lstat().  I think -P is so
>>>> broken that mtree on symlinks doesn't work at all and not using lstat()
>>>> would be safest.
>>>
>>> Hmmm... so I take it that this is actually the first step in many to
>>> fixing this underlying problem? I suppose I should be opening bugs for
>>> all of the itemized issues that you see in mtree(8) so someone can
>>> submit patches to fix the utility?
>>>
>>>> The patch has some style bugs.
>>>
>>> Please expound on this -- I want to improve my style (without having
>>> to rewrite the entire program of course) -- so that it conforms more
>>> to the projects overall style rules; of course there are some cases
>>> where I can't readily do that (like pkg_install -- ugh), but I'll do
>>> my best to make sure that the rules are withheld.
>>
>>    Just for the record, here's the latest patch that I submitted to
>> Bruce for this PR.
>
> Dog gone it; attached the wrong patch. This is the right patch..

    I've been sitting on this PR for a while and I'd like to wrap it
up and move on, if that's ok. Here's a patch with a more suitable
comment above the stat(2) call.
Thanks!
-Garrett
-------------- next part --------------
Index: usr.sbin/mtree/excludes.c
===================================================================
--- usr.sbin/mtree/excludes.c	(revision 213667)
+++ usr.sbin/mtree/excludes.c	(working copy)
@@ -30,9 +30,10 @@
 #include <sys/cdefs.h>
 __FBSDID("$FreeBSD$");
 
+#include <sys/queue.h>
+#include <sys/stat.h>
+#include <sys/time.h>		/* XXX for mtree.h */
 #include <sys/types.h>
-#include <sys/time.h>		/* XXX for mtree.h */
-#include <sys/queue.h>
 
 #include <err.h>
 #include <fnmatch.h>
@@ -63,11 +64,22 @@
 void
 read_excludes_file(const char *name)
 {
+	struct stat exclude_stat;
+	struct exclude *e;
 	FILE *fp;
 	char *line, *str;
-	struct exclude *e;
 	size_t len;
 
+	/* 
+	 * Make sure that the path we're dealing with points to a regular file,
+	 * because the exclude list should be a regular file, not a directory,
+	 * etc.
+	 */
+	if (stat(name, &exclude_stat) != 0)
+		err(EXIT_FAILURE, "stat: %s", name);
+	if (!S_ISREG(exclude_stat.st_mode))
+		errx(EXIT_FAILURE, "invalid exclude file: %s", name);
+
 	fp = fopen(name, "r");
 	if (fp == 0)
 		err(1, "%s", name);