Bug 98905 - [PATCH] devfs(8) segfaults if the ruleset doesn't end with a newline
Summary: [PATCH] devfs(8) segfaults if the ruleset doesn't end with a newline
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: unspecified
Hardware: Any Any
: Normal Affects Only Me
Assignee: Maxim Konovalov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-06-13 17:40 UTC by Fabian Keil
Modified: 2006-06-21 01:27 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Fabian Keil 2006-06-13 17:40:14 UTC
devfs segfaults if the ruleset doesn't end with a newline.

The man page doesn't say if rulesets are required to
end with newlines, but at least the code looks as if
they are not supposed to.

Please note that I'm not using vanilla sources.
One of the patches I use is Jeremie Le Hen's SSP patch,
but the devfs code is the original one and the problem
looks like a "normal" segfault to me.

Fix: 

With:
http://www.fabiankeil.de/sourcecode/freebsd/devfs.c.diff
I get:
fk@TP51 ~ $cat ~/test/pf-jail.rules
path pf hide
path pf unhidefk@TP51 ~ $
fk@TP51 ~ $sudo devfs rule -s 7 show
fk@TP51 ~ $sudo devfs rule -s 7 add - < ~/test/pf-jail.rules 
fk@TP51 ~ $sudo devfs rule -s 7 show
100 path pf hide
200 path pf unhide

Not sure if strlcpy is allowed in the base or if
that's the best solution though.
How-To-Repeat: fk@TP51 ~ $cat ~/test/pf-jail.rules
path pf hide
path pf unhidefk@TP51 ~ $
fk@TP51 ~ $sudo devfs rule -s 7 show
fk@TP51 ~ $sudo devfs rule -s 7 add - < ~/test/pf-jail.rules 
Segmentation fault: 11 (core dumped)
fk@TP51 ~ $sudo devfs rule -s 7 show
100 path pf hide
Comment 1 Maxim Konovalov 2006-06-14 12:32:01 UTC
Hi Fabian,

While your patch looks correct it is not clear for me why you removed
assert()?

Here is another version of the fix (copy&pasted):

===================================================================
RCS file: /home/ncvs/src/sbin/devfs/devfs.c,v
retrieving revision 1.5
diff -u -p -r1.5 devfs.c
--- devfs.c     22 Jan 2004 07:23:35 -0000      1.5
+++ devfs.c     14 Jun 2006 11:30:15 -0000
@@ -163,7 +163,7 @@ efgetln(FILE *fp, char **line)
                if (*line == NULL)
                        errx(1, "cannot allocate memory");
                memcpy(*line, cp, rv);
-               *line[rv] = '\0';
+               (*line)[rv] = '\0';
        }
        assert(rv == strlen(*line));
        return (rv);

-- 
Maxim Konovalov
Comment 2 Maxim Konovalov freebsd_committer 2006-06-14 12:47:56 UTC
State Changed
From-To: open->patched

Fixed in HEAD.  Thanks! 


Comment 3 Maxim Konovalov freebsd_committer 2006-06-14 12:47:56 UTC
Responsible Changed
From-To: freebsd-bugs->maxim

Feedbacks trap.
Comment 4 Fabian Keil 2006-06-14 13:21:56 UTC
Maxim Konovalov <maxim@macomnet.ru> wrote:

> While your patch looks correct it is not clear for me why you removed
> assert()?

It triggered a core dump and I falsely assumed it wasn't my fault.

I overlooked that strlcpy's return value it the length of (string + '\0'),
and although it doesn't seem to matter for devfs, the clean version
of my patch would be

--- devfs.c.orig        Tue Jun 13 17:54:48 2006
+++ devfs.c     Wed Jun 14 13:54:00 2006
@@ -162,8 +162,7 @@
                *line = malloc(rv + 1);
                if (*line == NULL)
                        errx(1, "cannot allocate memory");
-               memcpy(*line, cp, rv);
-               *line[rv] = '\0';
+               rv = strlcpy(*line, cp, rv+1) - 1;
        }
        assert(rv == strlen(*line));
        return (rv);
 
> Here is another version of the fix (copy&pasted):
> 
> ===================================================================
> RCS file: /home/ncvs/src/sbin/devfs/devfs.c,v
> retrieving revision 1.5
> diff -u -p -r1.5 devfs.c
> --- devfs.c     22 Jan 2004 07:23:35 -0000      1.5
> +++ devfs.c     14 Jun 2006 11:30:15 -0000
> @@ -163,7 +163,7 @@ efgetln(FILE *fp, char **line)
>                 if (*line == NULL)
>                         errx(1, "cannot allocate memory");
>                 memcpy(*line, cp, rv);
> -               *line[rv] = '\0';
> +               (*line)[rv] = '\0';
>         }
>         assert(rv == strlen(*line));
>         return (rv);

Works for me as well, and as this one doesn't bring
strlcpy in, it's probably a better solution.

Thanks for the fast response,

Fabian
-- 
http://www.fabiankeil.de/
Comment 5 Fabian Keil 2006-06-14 14:27:32 UTC
I'm CC'ing bug-followup@freebsd.org again,
I unintentionally broke the reply chain with my previous mail. 

Maxim Konovalov <maxim@macomnet.ru> wrote:

> On Wed, 14 Jun 2006, 14:14+0200, Fabian Keil wrote:
> 
> > Maxim Konovalov <maxim@macomnet.ru> wrote:
> >
> > > While your patch looks correct it is not clear for me why you
> > > removed assert()?
> >
> > It triggered a core dump and I falsely assumed it wasn't my fault.
> >
> > I overlooked that strlcpy's return value it the length of (string +
> > '\0'), and although it doesn't seem to matter for devfs, the clean
> > version of my patch would be
> >
> > --- devfs.c.orig        Tue Jun 13 17:54:48 2006
> > +++ devfs.c     Wed Jun 14 13:54:00 2006
> > @@ -162,8 +162,7 @@
> >                 *line = malloc(rv + 1);
> >                 if (*line == NULL)
> >                         errx(1, "cannot allocate memory");
> > -               memcpy(*line, cp, rv);
> > -               *line[rv] = '\0';
> > +               rv = strlcpy(*line, cp, rv+1) - 1;
> [...]
> 
> Still can't get why do you need to decrement rv.  strlcpy(3) returns a
> cp length, i.e. for the test case you provided:
> 
> 165                     rv = strlcpy(*line, cp, rv + 1);
> gdb% p rv
> $1 = 14
> gdb% n
> 167             assert(rv == strlen(*line));
> gdb% p rv
> $2 = 14
> gdb% p *line
> $3 = 0x8204140 "path pf unhide"

If I run my first flawed patch with printf's instead of the
assert I get:

fk@TP51 ~ $sudo devfs rule -s 6 add - < ~/test/pf-jail.rules
rv=12, strlen(*line)=12 line=path pf hide
rv=15, strlen(*line)=14 line=path pf unhide

Which wouldn't make it through the assert().
Maybe that's a difference between strlcpy in FreeBSD 6
and 7, but I also noticed that the strlcpy man page says:

|Also note that strlcpy() and strlcat() only operate on
|true ``C'' strings.  This means that for strlcpy() src must be NUL-termi-
|nated and for strlcat() both src and dst must be NUL-terminated.

Which would be another reason not to use my patch,
cp isn't NUL-terminated.

> > Works for me as well, and as this one doesn't bring
> > strlcpy in, it's probably a better solution.
> 
> I've just committed a patch withour rv decrementation.  Could you
> please verify it doesn't break anything in your environmen.  Thanks!

Unfortunately it doesn't. With a printf line before the assert I get:

fk@TP51 ~ $sudo devfs rule -s 6 add - < ~/test/pf-jail.rules
rv=12 strlen(*line)=12 *line=path pf hide
rv=15 strlen(*line)=14 *line=path pf unhide
Assertion failed: (rv == strlen(*line)), function efgetln, file /usr/src/sbin/devfs/devfs.c, line 168.
Abort trap: 6 (core dumped)

We could use:

--- devfs.c.orig        Tue Jun 13 17:54:48 2006
+++ devfs.c     Wed Jun 14 15:07:49 2006
@@ -162,8 +162,8 @@
                *line = malloc(rv + 1);
                if (*line == NULL)
                        errx(1, "cannot allocate memory");
-               memcpy(*line, cp, rv);
-               *line[rv] = '\0';
+                strlcpy(*line, cp, rv + 1);
+                rv = strlen(*line);
        }
        assert(rv == strlen(*line));
        return (rv);

which should work on your system as well, but of course
that would render the assert useless if the line doesn't
end with a new line and one might as well move the assert
inside the if block.

The patch you suggested in your first reply was working
on my system as well, so I suggest you use that one instead.

Fabian
-- 
http://www.fabiankeil.de/
Comment 6 Maxim Konovalov 2006-06-14 16:13:50 UTC
[...]
> The patch you suggested in your first reply was working
> on my system as well, so I suggest you use that one instead.

Yes, I finally got what was going on and committed my first version
instead.  Thanks!

-- 
Maxim Konovalov
Comment 7 Maxim Konovalov freebsd_committer 2006-06-21 01:27:00 UTC
State Changed
From-To: patched->closed

Merged to RELENG_6.