Bug 309

Summary: mdev -s should not abort on first error
Product: Busybox Reporter: Michael Tokarev <mjt+busybox>
Component: OtherAssignee: unassigned
Status: RESOLVED FIXED    
Severity: normal CC: busybox-cvs
Priority: P5    
Version: 1.13.x   
Target Milestone: ---   
Hardware: PC   
OS: Linux   
Host: Target:
Build:
Attachments: patch for 1.13 (with "warning: " added)

Description Michael Tokarev 2009-05-01 10:19:26 UTC
Currently, `mdev -s' (scan for all device nodes in /sys and populate /dev) aborts on first error found.  Most obvious one is when it tries to perform a chown for a device according to /etc/mdev.conf but is, for whatever reason, unable to find the user/group ID in question.  Sure thing this is configuration/operator error, but it is not a reason to have an unbootable system due to missing entries in /dev.  It should leave uid/gid at 0 and continue with the next device node.

Looking at the source I don't quite know how to process better: there's parse_chown_usergroup_or_die() routine that calls other get_foo() which will die if unable to find user/group ID.  So either all those routines should accept additional argument (fatal=yes/no) -- requires changing all callers, or they should be re-implemented for mdev -- which does not look right.
Comment 1 Denys Vlasenko 2009-05-02 14:31:27 UTC
Does this patch help?

http://busybox.net/downloads/fixes-1.14.0/busybox-1.14.0-mdev.patch
Comment 2 Michael Tokarev 2009-05-03 10:46:05 UTC
Yes that way it works much better.  The patch does not apply to 1.13 but both 1.14 and hand-applied 1.13 (2nd hunk) works as expected.  I'd also prefix the message with 'warning: ', because it is merely a warning now.

Thanks!
Comment 3 Michael Tokarev 2009-05-03 10:48:25 UTC
Created attachment 287 [details]
patch for 1.13 (with "warning: " added)
Comment 4 Michael Tokarev 2009-05-04 07:00:41 UTC
Ok, now that's.. interesting.

The patch triggers an old bug in mdev.  Here it is (only relevant parts are shown):

static void make_device(char *path, int delete) {
         char *dev_maj_min = path + strlen(path);

         if (!delete) {
               strcpy(dev_maj_min, "/dev");
                len = open_read_close(path, dev_maj_min + 1, 64);

Obviously it is writing past the `path' string, assuming it is in some large-enough buffer.  And it is when called from fileAction(), but not when called from mdev_main().  Before the change, make_device() were passed a scratch buffer.  Now we're passing value returned by xstrdup() which does not have anything past the string, and the above code corrupts malloc() data structures.

It looks like we don't need the strdup() since make_device(), while clobbers something past the string end, does not touch the string itself.  So the second hunk of the patch isn't needed.

Alternatively we can call load_firmware() before make_device().
Comment 5 Denys Vlasenko 2009-05-04 18:00:52 UTC
Reverted that chunk. I don't remeber (and can't find) how that "bright idea" slipped it...