Messages in this thread | | | Date | Tue, 16 May 2006 16:29:03 +0200 | From | Bernd Schmidt <> | Subject | Re: Please revert git commit 1ad3dcc0 |
| |
Andrew Morton wrote: > Bernd Schmidt <bernds_cb1@t-online.de> wrote: >> please revert 1ad3dcc0. That was a patch to the binfmt_flat loader, >> which was motivated by an LTP testcase which checks that execve returns >> EMFILE when the file descriptor table is full. >> >> The patch is buggy: the code now keeps file descriptors open for the >> executable and its libraries, which has confused at least one >> application. It's also unnecessary, since there is no code that uses >> the file descriptor, so the new EMFILE error return is totally artificial.
> I don't get it. The substance of the patch is > > + /* check file descriptor */ > + exec_fileno = get_unused_fd(); > + if (exec_fileno < 0) { > + ret = -EMFILE; > + goto err; > + } > + get_file(bprm->file); > + fd_install(exec_fileno, bprm->file); > > and that get_file() will be undone by exit(). Without this change we'll > forget to do file limit checking.
It's not the get_file that's the problem, it's the get_unused_fd and fd_install. These files are now open while the process lives and consume file descriptors. This does not happen with the ELF loader, which does
if (interpreter_type != INTERPRETER_AOUT) sys_close(elf_exec_fileno);
before transferring control to the application. So, fewer file descriptors are available for the app, and they start at a higher number.
Before the change, we didn't allocate or install a file descriptor, hence there wasn't any reason to return EMFILE. The spec at http://www.opengroup.org/onlinepubs/009695399/functions/exec.html doesn't list EMFILE as a possible error.
If you're unconvinced, then at the very least we need to add a sys_close call in the success path.
Bernd - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |