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
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
State Changed From-To: open->patched Fixed in HEAD. Thanks!
Responsible Changed From-To: freebsd-bugs->maxim Feedbacks trap.
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/
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/
[...] > 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
State Changed From-To: patched->closed Merged to RELENG_6.