Page MenuHomeFreeBSD

mdmfs: Fix many bugs in automatic md(4) creation.
ClosedPublic

Authored by brooks on Fri, Feb 8, 6:13 PM.

Details

Summary

This code allocated a correctly sized buffer, read past the end of the
source buffer, writing off the end of the target buffer, and then writing
a '\0' terminator past the end of the target buffer (in the wrong place).
It then leaked the buffer.

Switch to a statically sized buffer on the stack and update the source pointer and
length before use so the correct things are copied.

Fix a logic error in the checks that the format of the line is as
expected and move on out of an assert.

Remove an unneeded close(). fclose() closes the descriptor.

Found with: CheriABI
Obtained from: CheriBSD

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

brooks created this revision.Fri, Feb 8, 6:13 PM
markj added inline comments.Fri, Feb 8, 9:55 PM
sbin/mdmfs/mdmfs.c
484 ↗(On Diff #53694)

What if the line is just "md\n", i.e., there's no unit number? I think the second clause won't catch that case because linelen includes the newline character. That is, it looks like that should be linelen <= mdnamelen + 1.

brooks updated this revision to Diff 53713.Fri, Feb 8, 10:06 PM
  • Make sure there's at least space for one digit of a unit number.
brooks marked an inline comment as done.Fri, Feb 8, 10:06 PM
jhb added a comment.Sat, Feb 9, 12:19 AM

It's probably fine, but I wonder if it wouldn't be more readable to just use getline() instead. I think you'd end up with something like:

char *linebuf;
size_t linecap;
...
sfd = fdopen(...);
linebuf = NULL;
linecap = 0;
if (getline(&linebuf, &linecap, sfd) < mdnamelen ||
    strncmp(linebuf, mdname, mdnamelen) != 0)
        errx(1, "unexpected output from mdconfig (attach)");
ul = strtoul(linebuf + mdnamelen, &p, 10);
...
fclose(sfd);
free(linebuf);
sbin/mdmfs/mdmfs.c
490 ↗(On Diff #53713)

This assertion is now pointless.

500 ↗(On Diff #53713)

This close() is wrong, but harmless. fdopen() "takes" the fd and so fclose() already closes it.

kib added a comment.Sat, Feb 9, 3:27 AM

The buffer is not statically allocated, it is statically sized (in the revision summary).

brooks edited the summary of this revision. (Show Details)Mon, Feb 11, 7:02 PM
brooks edited the summary of this revision. (Show Details)
brooks updated this revision to Diff 53800.Mon, Feb 11, 7:05 PM
  • Remove obsolete and always true assert.
  • Remove pointless close(). fclose() closes the descriptor.
brooks marked 2 inline comments as done.Mon, Feb 11, 7:05 PM
jhb accepted this revision.Mon, Feb 11, 8:14 PM
This revision is now accepted and ready to land.Mon, Feb 11, 8:14 PM
markj accepted this revision.Mon, Feb 11, 8:48 PM
This revision was automatically updated to reflect the committed changes.