Ingo Schwarze wrote:
> Hi,
>
> when p->p_rlimit[RLIMIT_DATA].rlim_cur < ptoa(p->p_vmspace->vm_dused),
> the following comparison fails because the subtraction wraps around
> to huge positive values.
>
> I noticed this because i tried to do systematic testing of specific
> malloc failure codepaths. But whenever i used setrlimit(2) to set
> a low RLIMIT_DATA, a subsequent malloc(3) with a big argument always
> succeeded.
>
> It may be unsusual for a program to lower its own RLIMIT_DATA, and
> i'm neither sure whether there are other use cases besides testing
> nor whether there are other ways to arrive in a situation with
> p->p_rlimit[RLIMIT_DATA].rlim_cur < ptoa(p->p_vmspace->vm_dused)
> besides late setrlimit(2) to a low value.
>
> But even if there are no use cases besides testing, do we want
> to have an unsigned comparison in the kernel that can wrap around?
>
> I can hardly believe i'm sending such a diff... :-o
>
> I'm running it right now. So far, i noticed no regressions,
> and testing of malloc failure codepaths has become much easier.
> I consider that useful to make out software better.
>
> What do you think?
Agreed that we need to check for wraparound here.
For some reason the
p->p_rlimit[RLIMIT_DATA].rlim_cur - size < ptoa(p->p_vmspace->vm_dused)
confuses me, although the operands compared to the original check are
just re-arranged.
How about writing it like this?
ptoa(p->p_vmspace->vm_used) > p->p_rlimit[RLIMIT_DATA].rlim_cur ||
size > (p->p_rlimit[RLIMIT_DATA].rlim_cur - ptoa(p->p_vmspace->vm_dused))
> Yours,
> Ingo
>
>
> Index: uvm/uvm_mmap.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_mmap.c,v
> retrieving revision 1.130
> diff -u -p -r1.130 uvm_mmap.c
> --- uvm/uvm_mmap.c 1 Jun 2016 04:53:54 -0000 1.130
> +++ uvm/uvm_mmap.c 2 Jun 2016 01:23:20 -0000
> @@ -526,8 +526,9 @@ sys_mmap(struct proc *p, void *v, regist
> }
> if ((flags & MAP_ANON) != 0 ||
> ((flags & MAP_PRIVATE) != 0 && (prot & PROT_WRITE) != 0)) {
> - if (size >
> - (p->p_rlimit[RLIMIT_DATA].rlim_cur -
> ptoa(p->p_vmspace->vm_dused))) {
> + if (p->p_rlimit[RLIMIT_DATA].rlim_cur < size ||
> + p->p_rlimit[RLIMIT_DATA].rlim_cur - size <
> + ptoa(p->p_vmspace->vm_dused)) {
> error = ENOMEM;
> goto out;
> }
> @@ -545,8 +546,9 @@ is_anon: /* label for SunOS style /dev/z
>
> if ((flags & MAP_ANON) != 0 ||
> ((flags & MAP_PRIVATE) != 0 && (prot & PROT_WRITE) != 0)) {
> - if (size >
> - (p->p_rlimit[RLIMIT_DATA].rlim_cur -
> ptoa(p->p_vmspace->vm_dused))) {
> + if (p->p_rlimit[RLIMIT_DATA].rlim_cur < size ||
> + p->p_rlimit[RLIMIT_DATA].rlim_cur - size <
> + ptoa(p->p_vmspace->vm_dused)) {
> return ENOMEM;
> }
> }
>
