| Message ID | 20170320193558.GB26547@altlinux.org |
|---|---|
| State | New |
| Headers | show |
On Mon, Mar 20, 2017 at 3:35 PM, Dmitry V. Levin <ldv@altlinux.org> wrote: > > I understand the patch is trivial, but anyway, there is a bug and it has > to be fixed. > If there are no comments, I'd push it rather than go on with these ping > reposts. We have a collective problem where nobody feels empowered to say "yes" to patches. Also, you found this bug by fault injection -- do you have reason to believe that this mprotect call can actually fail? If so, under what circumstances, and how bad are the consequences? > + { > + /* Change protection on the excess portion to disallow all access; > + the portions we do not remap later will be inaccessible as if > + unallocated. Then jump into the normal segment-mapping loop to > + handle the portion of the segment past the end of the file > + mapping. */ > + int rc; > + rc = __mprotect ((caddr_t) (l->l_addr + c->mapend), > + loadcmds[nloadcmds - 1].mapstart - c->mapend, > + PROT_NONE); > + if (__glibc_unlikely (rc < 0)) > + return DL_MAP_SEGMENTS_ERROR_MPROTECT; > + } The variable 'rc' appears to be unnecessary. Why not just if (__glibc_unlikely (__mprotect (...) < 0)) return DL_MAP_SEGMENTS_ERROR_MPROTECT; ?
On Mon, Mar 20, 2017 at 03:58:22PM -0400, Zack Weinberg wrote: > On Mon, Mar 20, 2017 at 3:35 PM, Dmitry V. Levin <ldv@altlinux.org> wrote: > > > > I understand the patch is trivial, but anyway, there is a bug and it has > > to be fixed. > > If there are no comments, I'd push it rather than go on with these ping > > reposts. > > We have a collective problem where nobody feels empowered to say "yes" > to patches. Yes, we have this problem, unfortunately. > Also, you found this bug by fault injection -- do you have reason to > believe that this mprotect call can actually fail? If so, under what > circumstances, and how bad are the consequences? Every mprotect call that increases memory fragmentation can legitimately fail with ENOMEM, the fault injection technique is just a very easy way to reproduce the error. > > + { > > + /* Change protection on the excess portion to disallow all access; > > + the portions we do not remap later will be inaccessible as if > > + unallocated. Then jump into the normal segment-mapping loop to > > + handle the portion of the segment past the end of the file > > + mapping. */ > > + int rc; > > + rc = __mprotect ((caddr_t) (l->l_addr + c->mapend), > > + loadcmds[nloadcmds - 1].mapstart - c->mapend, > > + PROT_NONE); > > + if (__glibc_unlikely (rc < 0)) > > + return DL_MAP_SEGMENTS_ERROR_MPROTECT; > > + } > > The variable 'rc' appears to be unnecessary. Why not just > > if (__glibc_unlikely (__mprotect (...) < 0)) > return DL_MAP_SEGMENTS_ERROR_MPROTECT; > > ? I want to keep the code readable. If I did this, the line would get too long and I'd have to cut loadcmds[nloadcmds - 1].mapstart - c->mapend into pieces making it harder to comprehend.
On Mon, Mar 20, 2017 at 4:27 PM, Dmitry V. Levin <ldv@altlinux.org> wrote: > On Mon, Mar 20, 2017 at 03:58:22PM -0400, Zack Weinberg wrote: >> Also, you found this bug by fault injection -- do you have reason to >> believe that this mprotect call can actually fail? If so, under what >> circumstances, and how bad are the consequences? > > Every mprotect call that increases memory fragmentation can legitimately > fail with ENOMEM, the fault injection technique is just a very easy way > to reproduce the error. OK, I'll accept that as sufficient reason to go forward with the patch. >> The variable 'rc' appears to be unnecessary. Why not just >> >> if (__glibc_unlikely (__mprotect (...) < 0)) >> return DL_MAP_SEGMENTS_ERROR_MPROTECT; >> >> ? > > I want to keep the code readable. If I did this, the line would get > too long and I'd have to cut > loadcmds[nloadcmds - 1].mapstart - c->mapend > into pieces making it harder to comprehend. You could do it like this: { /* Change protection on the excess portion to disallow all access; the portions we do not remap later will be inaccessible as if unallocated. Then jump into the normal segment-mapping loop to handle the portion of the segment past the end of the file mapping. */ if (__glibc_unlikely (__mprotect ((caddr_t) (l->l_addr + c->mapend), loadcmds[nloadcmds - 1].mapstart - c->mapend, PROT_NONE) < 0)) return DL_MAP_SEGMENTS_ERROR_MPROTECT; } with the arguments to mprotect not any further rightward. zw
diff --git a/elf/dl-map-segments.h b/elf/dl-map-segments.h index e583f64..3dc030b 100644 --- a/elf/dl-map-segments.h +++ b/elf/dl-map-segments.h @@ -64,14 +64,19 @@ _dl_map_segments (struct link_map *l, int fd, l->l_addr = l->l_map_start - c->mapstart; if (has_holes) - /* Change protection on the excess portion to disallow all access; - the portions we do not remap later will be inaccessible as if - unallocated. Then jump into the normal segment-mapping loop to - handle the portion of the segment past the end of the file - mapping. */ - __mprotect ((caddr_t) (l->l_addr + c->mapend), - loadcmds[nloadcmds - 1].mapstart - c->mapend, - PROT_NONE); + { + /* Change protection on the excess portion to disallow all access; + the portions we do not remap later will be inaccessible as if + unallocated. Then jump into the normal segment-mapping loop to + handle the portion of the segment past the end of the file + mapping. */ + int rc; + rc = __mprotect ((caddr_t) (l->l_addr + c->mapend), + loadcmds[nloadcmds - 1].mapstart - c->mapend, + PROT_NONE); + if (__glibc_unlikely (rc < 0)) + return DL_MAP_SEGMENTS_ERROR_MPROTECT; + } l->l_contiguous = 1;