Re: options MODULAR improvements phase 1.0

[jnemeth%victoria.tc.ca@localhost (John Nemeth)]

On Oct 24, 12:53pm, Matthew Mondor wrote:
} On Tue, 2 Jun 2009 18:48:53 -0700
} jnemeth%victoria.tc.ca@localhost (John Nemeth) wrote:
} 
} > diff [...]
} 
} It's possible that I misinterpreted the code, but by a quick look I
} think I've seen the following issue:
} 
} It's possible in module_load_plist_file() for a failure in vn_stat() or
} vn_open() to set error and jump to out1, which might set *basep to NULL
} (base before kmem_zalloc()) or for a failure of vn_rdrw() to cause the
} buffer to be kmem_free()ed and base set again to NULL (and returned in
} *basep);  However, the caller (I can't see its function name by the
} diff) seems to explicitely still kmem_free(plist) in case of error
} without a NULL check, and kmem_free(9) suggests freeing NULL is
} illegal...

     The caller is near the end of module_do_load().  Anyways, good
catch.  There was a second issue in that plist wasn't freed in the
non-error case (mmm...  kernel memory leaks).  Anyways, that code now
reads:

        /*
         * Load and process <module>.prop if it exists.
         */
        if (mod->mod_source == MODULE_SOURCE_FILESYS) {
                error = module_load_plist_file(path, nochroot, &plist,
                    &plistlen);
                if (error != 0) {
                        module_print("plist load returned error %d for `%s'",
                            error, path);
                } else {
                        filedict = prop_dictionary_internalize(plist);
                        if (filedict == NULL) {
                                error = EINVAL;
                        }
                }
                if (plist != NULL) {
                        kmem_free(plist, PAGE_SIZE);
                }
                if ((error != 0) && (error != ENOENT)) {
                        goto fail;
                }
        }

     I've also changed the kmem_zalloc() to kmem_alloc() (no need to
waste time zeroing memory that won't be passed to userland).  The code
segment (in module_load_plist_file()) now reads:

        base = kmem_alloc(PAGE_SIZE, KM_SLEEP);
        if (base == NULL) {
                error = ENOMEM;
                goto out;
        }

        error = vn_rdwr(UIO_READ, nd.ni_vp, base, sb.st_size, 0,
            UIO_SYSSPACE, IO_NODELOCKED, curlwp->l_cred, &resid, curlwp);
        *((uint8_t *)base + sb.st_size) = '\0';

}-- End of excerpt from Matthew Mondor