Summary: | [PATCH] devfs(8) segfaults if the ruleset doesn't end with a newline | ||
---|---|---|---|
Product: | Base System | Reporter: | Fabian Keil <freebsd-listen> |
Component: | bin | Assignee: | Maxim Konovalov <maxim> |
Status: | Closed FIXED | ||
Severity: | Affects Only Me | ||
Priority: | Normal | ||
Version: | Unspecified | ||
Hardware: | Any | ||
OS: | Any |
Description
Fabian Keil
2006-06-13 17:40:14 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 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. |