Differential D5112
dlopen(3) crashes when opening empty .so Authored by sobomax on Jan 29 2016, 5:34 AM. Tags None Referenced Files
Subscribers
Details This seems like a very trivial bug that should have been squashed a long time ago, but for some reason it was not. Basically, without this change dlopen(3)'ing an empty .so file would just cause application to dump core with SIGSEGV. Make sure the file has enough data for at least the ELF header before mmap'ing it. ATF test case is attached.
Diff Detail
Event TimelineComment Actions Small test case: #include <sys/stat.h>
#include <dlfcn.h>
#include <err.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
int main()
{
char tempname[] = "/tmp/temp.XXXXXX";
char *fname, *soname;
int fd;
void *dlh;
fname = mktemp(tempname);
if (fname == NULL) {
err(1, "mktemp");
}
asprintf(&soname, "%s.so", fname);
fd = open(soname, O_WRONLY | O_CREAT | O_TRUNC, DEFFILEMODE);
if (fd == -1) {
err(1, "open(%s)", soname);
}
printf("%s %d\n", soname, fd);
close(fd);
dlh = dlopen(soname, RTLD_LAZY);
if (dlh != NULL) {
dlclose(dlh);
} else {
printf("%s\n", dlerror());
}
unlink(soname);
}Comment Actions Please upload patches with more context in the future (-U9999). It seems like mmap(2) *ought* to fail on zero-length files. Shortly after the new fstat check is an mmap failure check: 331 hdr = mmap(NULL, PAGE_SIZE, PROT_READ, MAP_PRIVATE | MAP_PREFAULT_READ,
332 fd, 0);
333 if (hdr == (Elf_Ehdr *)MAP_FAILED) {
334 _rtld_error("%s: read error: %s", path, rtld_strerror(errno));
335 return (NULL);
336 }Why doesn't that fail?
Comment Actions It seems like vm_mmap_vnode should fail if the vnode va_size is zero. No non-zero request size is satisfiable with a zero-length file. Comment Actions I don't know. It returns pointer, but attempt to read few bytes offset from that pointer causes SEGV. In any case that would be a separate issue, technically file can have say only 1 byte, so mmap would not fail but reading beyond that 1 byte would be just as incorrect, although probably not detectable since memory mapping is done in PAGE_SIZE chunks AFAIK. -Max Comment Actions Even a 1-byte file would be valid to map PAGE_SIZE (the remaining contents of the partial page should be zero). It is just the zero length file where mapping any non-zero length ought to fail. (And zero-length mmap requests are invalid by definition, so it is invalid to mmap zero-length files at all.) A 1-byte file should successfully mmap the partial page (the program can read the full PAGE_SIZE, of course), then fail to parse as ELF and dlopen fails as expected (no segfault). A zero byte file should failed to mmap PAGE_SIZE bytes and return MAP_FAILED. Comment Actions Where does this documented? I see nothing on manpage and internets don't return anything concise. Our mmap() definitely does not do it (10.1, 11.0) -Max
Comment Actions Ok, Jilles says reservations larger than file size are allowed. The intent is to allow extensions to the file without having to find a new address space for mmapping. Access to whole pages beyond EOF should raise SIGBUS. Comment Actions Well, I am catching only that (sigaction(SIGSEGV), so it must be it: $ ./dlopen_empty_test dlopen_empty_test
Comment Actions Ok. The fault should actually be raising SIGBUS, not SIGSEGV, but that's a wholly separate bug. The fix for rtld and test cases look good, thanks.
Comment Actions Thanks. Although I would remove the NULL check. Also, is not the signal SIGBUS as opposed to SIGSEGV? Just for commit message purposes. Comment Actions mjd, thanks, I'll just pass that info down. I think for execve path, it's safe to assume that the kernel already has done lot of legwork to make sure the file has at least some ELF-foo in it. As for the sizeof(Elf_Ehdr) check, yes not ideal, but that should be enough to make sure we can read off the elf magic in the IS_ELF(), which is actually where we get SIGSEGV now. Comment Actions Ouch, I see that stat buffer indeed can be null. I guess it is acceptable to omit the sanity check. Comment Actions It's definitely SIGSEGV, see the test case and my earlier conversation with cem. Could be some separate issue in mmap(2). -Max | ||||||||||||||||||||||||||||||||||||||||||||||||||||