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