alpha: correct handling of negative *rlimit() args besides -1
Checks
| Context |
Check |
Description |
| dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
| dj/TryBot-32bit |
success
|
Build for i686
|
Commit Message
The generic version of RLIM_INFINITY in Linux is equal to (rlim_t)-1,
which is equal to ULLONG_MAX. On alpha however it is instead defined as
0x7ffffffffffffffful. This was special-cased in 0d0bc78 [BZ #22648] but
it specifically used an equality check.
There is a cpython test case test_prlimit_refcount which calls
setrlimit() with { -2, -2 } as arguments rather than the usual -1, it
therefore fails the equality test and is treated as a large arbitrary
positive value past the maximum of RLIM_INFINITY and fails with EPERM.
This patch changes the behavior of the *rlimit() calls to treat all
integers between 0x7ffffffffffffffful and (rlim_t)-1 as (rlim_t)-1,
i.e., RLIM_INFINITY.
---
sysdeps/unix/sysv/linux/alpha/getrlimit64.c | 4 ++--
sysdeps/unix/sysv/linux/alpha/setrlimit64.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
Comments
On 10/7/22 22:45, Matt Turner via Libc-alpha wrote:
> The generic version of RLIM_INFINITY in Linux is equal to (rlim_t)-1,
> which is equal to ULLONG_MAX. On alpha however it is instead defined as
> 0x7ffffffffffffffful. This was special-cased in 0d0bc78 [BZ #22648] but
> it specifically used an equality check.
>
> There is a cpython test case test_prlimit_refcount which calls
> setrlimit() with { -2, -2 } as arguments rather than the usual -1, it
> therefore fails the equality test and is treated as a large arbitrary
> positive value past the maximum of RLIM_INFINITY and fails with EPERM.
> This patch changes the behavior of the *rlimit() calls to treat all
> integers between 0x7ffffffffffffffful and (rlim_t)-1 as (rlim_t)-1,
> i.e., RLIM_INFINITY.
> ---
> sysdeps/unix/sysv/linux/alpha/getrlimit64.c | 4 ++--
> sysdeps/unix/sysv/linux/alpha/setrlimit64.c | 4 ++--
Is MIPS affected by the same problem?
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/sysdeps/unix/sysv/linux/alpha/getrlimit64.c b/sysdeps/unix/sysv/linux/alpha/getrlimit64.c
> index c195f5b55c..40f3e6bdff 100644
> --- a/sysdeps/unix/sysv/linux/alpha/getrlimit64.c
> +++ b/sysdeps/unix/sysv/linux/alpha/getrlimit64.c
> @@ -38,11 +38,11 @@ __old_getrlimit64 (enum __rlimit_resource resource,
> if (__getrlimit64 (resource, &krlimits) < 0)
> return -1;
>
> - if (krlimits.rlim_cur == RLIM64_INFINITY)
> + if (krlimits.rlim_cur >= OLD_RLIM64_INFINITY)
> rlimits->rlim_cur = OLD_RLIM64_INFINITY;
> else
> rlimits->rlim_cur = krlimits.rlim_cur;
> - if (krlimits.rlim_max == RLIM64_INFINITY)
> + if (krlimits.rlim_max >= OLD_RLIM64_INFINITY)
> rlimits->rlim_max = OLD_RLIM64_INFINITY;
> else
> rlimits->rlim_max = krlimits.rlim_max;
> diff --git a/sysdeps/unix/sysv/linux/alpha/setrlimit64.c b/sysdeps/unix/sysv/linux/alpha/setrlimit64.c
> index 421616ed20..4e88540a48 100644
> --- a/sysdeps/unix/sysv/linux/alpha/setrlimit64.c
> +++ b/sysdeps/unix/sysv/linux/alpha/setrlimit64.c
> @@ -35,11 +35,11 @@ __old_setrlimit64 (enum __rlimit_resource resource,
> {
> struct rlimit64 krlimits;
>
> - if (rlimits->rlim_cur == OLD_RLIM64_INFINITY)
> + if (rlimits->rlim_cur >= OLD_RLIM64_INFINITY)
> krlimits.rlim_cur = RLIM64_INFINITY;
> else
> krlimits.rlim_cur = rlimits->rlim_cur;
> - if (rlimits->rlim_max == OLD_RLIM64_INFINITY)
> + if (rlimits->rlim_max >= OLD_RLIM64_INFINITY)
> krlimits.rlim_max = RLIM64_INFINITY;
> else
> krlimits.rlim_max = rlimits->rlim_max;
On Sun, Oct 9, 2022 at 8:51 PM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 10/7/22 22:45, Matt Turner via Libc-alpha wrote:
> > The generic version of RLIM_INFINITY in Linux is equal to (rlim_t)-1,
> > which is equal to ULLONG_MAX. On alpha however it is instead defined as
> > 0x7ffffffffffffffful. This was special-cased in 0d0bc78 [BZ #22648] but
> > it specifically used an equality check.
> >
> > There is a cpython test case test_prlimit_refcount which calls
> > setrlimit() with { -2, -2 } as arguments rather than the usual -1, it
> > therefore fails the equality test and is treated as a large arbitrary
> > positive value past the maximum of RLIM_INFINITY and fails with EPERM.
> > This patch changes the behavior of the *rlimit() calls to treat all
> > integers between 0x7ffffffffffffffful and (rlim_t)-1 as (rlim_t)-1,
> > i.e., RLIM_INFINITY.
> > ---
> > sysdeps/unix/sysv/linux/alpha/getrlimit64.c | 4 ++--
> > sysdeps/unix/sysv/linux/alpha/setrlimit64.c | 4 ++--
>
> Is MIPS affected by the same problem?
Looks to me like it is, but I don't have a mips system running to
confirm. If this patch is accepted, I'll send a follow-up for mips.
Hi,
On 2022-10-07 22:45, Matt Turner via Libc-alpha wrote:
> The generic version of RLIM_INFINITY in Linux is equal to (rlim_t)-1,
> which is equal to ULLONG_MAX. On alpha however it is instead defined as
> 0x7ffffffffffffffful. This was special-cased in 0d0bc78 [BZ #22648] but
> it specifically used an equality check.
I am not sure this commit is giving the full picture, commits around
should also be checked to understand it.
> There is a cpython test case test_prlimit_refcount which calls
> setrlimit() with { -2, -2 } as arguments rather than the usual -1, it
> therefore fails the equality test and is treated as a large arbitrary
> positive value past the maximum of RLIM_INFINITY and fails with EPERM.
> This patch changes the behavior of the *rlimit() calls to treat all
> integers between 0x7ffffffffffffffful and (rlim_t)-1 as (rlim_t)-1,
> i.e., RLIM_INFINITY.
Basically on alpha, the glibc API is now identical to the prlimit64 API,
which means there is a dead zone with invalid values from
0x8000000000000000ul to 0xfffffffffffffffeul. The kernel returns EPERM
for values in this range.
You suggestion is to consider values is this zone as infinity. I have
mixed feeling about that. From the setrlimit() side it looks like the
correct thing to do. But this breaks the assumption that calling
getrlimit() after a successful setrlimit() call will return the same
value.
> diff --git a/sysdeps/unix/sysv/linux/alpha/getrlimit64.c b/sysdeps/unix/sysv/linux/alpha/getrlimit64.c
> index c195f5b55c..40f3e6bdff 100644
> --- a/sysdeps/unix/sysv/linux/alpha/getrlimit64.c
> +++ b/sysdeps/unix/sysv/linux/alpha/getrlimit64.c
> @@ -38,11 +38,11 @@ __old_getrlimit64 (enum __rlimit_resource resource,
> if (__getrlimit64 (resource, &krlimits) < 0)
> return -1;
>
> - if (krlimits.rlim_cur == RLIM64_INFINITY)
> + if (krlimits.rlim_cur >= OLD_RLIM64_INFINITY)
> rlimits->rlim_cur = OLD_RLIM64_INFINITY;
> else
> rlimits->rlim_cur = krlimits.rlim_cur;
> - if (krlimits.rlim_max == RLIM64_INFINITY)
> + if (krlimits.rlim_max >= OLD_RLIM64_INFINITY)
> rlimits->rlim_max = OLD_RLIM64_INFINITY;
> else
> rlimits->rlim_max = krlimits.rlim_max;
That said, I do not understand the change there. It is done on the
*compat* symbol which still uses the old glibc API definition. There we
want to keep doing the exact reverse operations as in the
rlim_to_rlim64() kernel function.
> diff --git a/sysdeps/unix/sysv/linux/alpha/setrlimit64.c b/sysdeps/unix/sysv/linux/alpha/setrlimit64.c
> index 421616ed20..4e88540a48 100644
> --- a/sysdeps/unix/sysv/linux/alpha/setrlimit64.c
> +++ b/sysdeps/unix/sysv/linux/alpha/setrlimit64.c
> @@ -35,11 +35,11 @@ __old_setrlimit64 (enum __rlimit_resource resource,
> {
> struct rlimit64 krlimits;
>
> - if (rlimits->rlim_cur == OLD_RLIM64_INFINITY)
> + if (rlimits->rlim_cur >= OLD_RLIM64_INFINITY)
> krlimits.rlim_cur = RLIM64_INFINITY;
> else
> krlimits.rlim_cur = rlimits->rlim_cur;
> - if (rlimits->rlim_max == OLD_RLIM64_INFINITY)
> + if (rlimits->rlim_max >= OLD_RLIM64_INFINITY)
> krlimits.rlim_max = RLIM64_INFINITY;
> else
> krlimits.rlim_max = rlimits->rlim_max;
Ditto here we want to do the reverse operations as the rlim64_to_rlim()
kernel function.
Hi Aurelien, I came up with the idea for this originally. Matt noticed
that it had stalled and asked me to check back in.
> On 2022-10-07 22:45, Matt Turner via Libc-alpha wrote:
> > The generic version of RLIM_INFINITY in Linux is equal to (rlim_t)-1,
> > which is equal to ULLONG_MAX. On alpha however it is instead defined as
> > 0x7ffffffffffffffful. This was special-cased in 0d0bc78 [BZ #22648] but
> > it specifically used an equality check.
>
> I am not sure this commit is giving the full picture, commits around
> should also be checked to understand it.
Can you elaborate here? This was my understanding based on what I read,
but you are the original author, so your perspective will surely be more
complete than mine.
> > There is a cpython test case test_prlimit_refcount which calls
> > setrlimit() with { -2, -2 } as arguments rather than the usual -1, it
> > therefore fails the equality test and is treated as a large arbitrary
> > positive value past the maximum of RLIM_INFINITY and fails with EPERM.
> > This patch changes the behavior of the *rlimit() calls to treat all
> > integers between 0x7ffffffffffffffful and (rlim_t)-1 as (rlim_t)-1,
> > i.e., RLIM_INFINITY.
>
> Basically on alpha, the glibc API is now identical to the prlimit64
> API,
> which means there is a dead zone with invalid values from
> 0x8000000000000000ul to 0xfffffffffffffffeul. The kernel returns EPERM
> for values in this range.
>
> You suggestion is to consider values is this zone as infinity. I have
> mixed feeling about that. From the setrlimit() side it looks like the
> correct thing to do. But this breaks the assumption that calling
> getrlimit() after a successful setrlimit() call will return the same
> value.
Is this behavior specified one way or the other? Alternatively, is the
Python unit test making an assumption that is not guaranteed (that
calling setrlimit() with a negative value behaves the same way as
calling it specifically with RLIM_INFINITY)? If this is Python's
mistake, that can be corrected there. The test in question:
https://github.com/python/cpython/blob/main/Lib/test/test_resource.py#L163-L175
> > diff --git a/sysdeps/unix/sysv/linux/alpha/getrlimit64.c b/sysdeps/unix/sysv/linux/alpha/getrlimit64.c
> > index c195f5b55c..40f3e6bdff 100644
> > --- a/sysdeps/unix/sysv/linux/alpha/getrlimit64.c
> > +++ b/sysdeps/unix/sysv/linux/alpha/getrlimit64.c
> > @@ -38,11 +38,11 @@ __old_getrlimit64 (enum __rlimit_resource resource,
> > if (__getrlimit64 (resource, &krlimits) < 0)
> > return -1;
> >
> > - if (krlimits.rlim_cur == RLIM64_INFINITY)
> > + if (krlimits.rlim_cur >= OLD_RLIM64_INFINITY)
> > rlimits->rlim_cur = OLD_RLIM64_INFINITY;
> > else
> > rlimits->rlim_cur = krlimits.rlim_cur;
> > - if (krlimits.rlim_max == RLIM64_INFINITY)
> > + if (krlimits.rlim_max >= OLD_RLIM64_INFINITY)
> > rlimits->rlim_max = OLD_RLIM64_INFINITY;
> > else
> > rlimits->rlim_max = krlimits.rlim_max;
>
> That said, I do not understand the change there. It is done on the
> *compat* symbol which still uses the old glibc API definition. There we
> want to keep doing the exact reverse operations as in the
> rlim_to_rlim64() kernel function.
>
> > diff --git a/sysdeps/unix/sysv/linux/alpha/setrlimit64.c b/sysdeps/unix/sysv/linux/alpha/setrlimit64.c
> > index 421616ed20..4e88540a48 100644
> > --- a/sysdeps/unix/sysv/linux/alpha/setrlimit64.c
> > +++ b/sysdeps/unix/sysv/linux/alpha/setrlimit64.c
> > @@ -35,11 +35,11 @@ __old_setrlimit64 (enum __rlimit_resource resource,
> > {
> > struct rlimit64 krlimits;
> >
> > - if (rlimits->rlim_cur == OLD_RLIM64_INFINITY)
> > + if (rlimits->rlim_cur >= OLD_RLIM64_INFINITY)
> > krlimits.rlim_cur = RLIM64_INFINITY;
> > else
> > krlimits.rlim_cur = rlimits->rlim_cur;
> > - if (rlimits->rlim_max == OLD_RLIM64_INFINITY)
> > + if (rlimits->rlim_max >= OLD_RLIM64_INFINITY)
> > krlimits.rlim_max = RLIM64_INFINITY;
> > else
> > krlimits.rlim_max = rlimits->rlim_max;
>
> Ditto here we want to do the reverse operations as the rlim64_to_rlim()
> kernel function.
I don't quite understand where else the change would go. We don't want
to be touching the generic implementations do we? Or are you saying
this should actually be going in the kernel and not glibc?
On 2023-02-14 14:38, matoro wrote:
> Hi Aurelien, I came up with the idea for this originally. Matt noticed
> that it had stalled and asked me to check back in.
>
>> On 2022-10-07 22:45, Matt Turner via Libc-alpha wrote:
>> > The generic version of RLIM_INFINITY in Linux is equal to (rlim_t)-1,
>> > which is equal to ULLONG_MAX. On alpha however it is instead defined as
>> > 0x7ffffffffffffffful. This was special-cased in 0d0bc78 [BZ #22648] but
>> > it specifically used an equality check.
>>
>> I am not sure this commit is giving the full picture, commits around
>> should also be checked to understand it.
>
> Can you elaborate here? This was my understanding based on what I
> read, but you are the original author, so your perspective will surely
> be more complete than mine.
>
>> > There is a cpython test case test_prlimit_refcount which calls
>> > setrlimit() with { -2, -2 } as arguments rather than the usual -1, it
>> > therefore fails the equality test and is treated as a large arbitrary
>> > positive value past the maximum of RLIM_INFINITY and fails with EPERM.
>> > This patch changes the behavior of the *rlimit() calls to treat all
>> > integers between 0x7ffffffffffffffful and (rlim_t)-1 as (rlim_t)-1,
>> > i.e., RLIM_INFINITY.
>>
>> Basically on alpha, the glibc API is now identical to the prlimit64
>> API,
>> which means there is a dead zone with invalid values from
>> 0x8000000000000000ul to 0xfffffffffffffffeul. The kernel returns EPERM
>> for values in this range.
>>
>> You suggestion is to consider values is this zone as infinity. I have
>> mixed feeling about that. From the setrlimit() side it looks like the
>> correct thing to do. But this breaks the assumption that calling
>> getrlimit() after a successful setrlimit() call will return the same
>> value.
>
> Is this behavior specified one way or the other? Alternatively, is the
> Python unit test making an assumption that is not guaranteed (that
> calling setrlimit() with a negative value behaves the same way as
> calling it specifically with RLIM_INFINITY)? If this is Python's
> mistake, that can be corrected there. The test in question:
> https://github.com/python/cpython/blob/main/Lib/test/test_resource.py#L163-L175
>
>> > diff --git a/sysdeps/unix/sysv/linux/alpha/getrlimit64.c b/sysdeps/unix/sysv/linux/alpha/getrlimit64.c
>> > index c195f5b55c..40f3e6bdff 100644
>> > --- a/sysdeps/unix/sysv/linux/alpha/getrlimit64.c
>> > +++ b/sysdeps/unix/sysv/linux/alpha/getrlimit64.c
>> > @@ -38,11 +38,11 @@ __old_getrlimit64 (enum __rlimit_resource resource,
>> > if (__getrlimit64 (resource, &krlimits) < 0)
>> > return -1;
>> >
>> > - if (krlimits.rlim_cur == RLIM64_INFINITY)
>> > + if (krlimits.rlim_cur >= OLD_RLIM64_INFINITY)
>> > rlimits->rlim_cur = OLD_RLIM64_INFINITY;
>> > else
>> > rlimits->rlim_cur = krlimits.rlim_cur;
>> > - if (krlimits.rlim_max == RLIM64_INFINITY)
>> > + if (krlimits.rlim_max >= OLD_RLIM64_INFINITY)
>> > rlimits->rlim_max = OLD_RLIM64_INFINITY;
>> > else
>> > rlimits->rlim_max = krlimits.rlim_max;
>>
>> That said, I do not understand the change there. It is done on the
>> *compat* symbol which still uses the old glibc API definition. There
>> we
>> want to keep doing the exact reverse operations as in the
>> rlim_to_rlim64() kernel function.
>>
>> > diff --git a/sysdeps/unix/sysv/linux/alpha/setrlimit64.c b/sysdeps/unix/sysv/linux/alpha/setrlimit64.c
>> > index 421616ed20..4e88540a48 100644
>> > --- a/sysdeps/unix/sysv/linux/alpha/setrlimit64.c
>> > +++ b/sysdeps/unix/sysv/linux/alpha/setrlimit64.c
>> > @@ -35,11 +35,11 @@ __old_setrlimit64 (enum __rlimit_resource resource,
>> > {
>> > struct rlimit64 krlimits;
>> >
>> > - if (rlimits->rlim_cur == OLD_RLIM64_INFINITY)
>> > + if (rlimits->rlim_cur >= OLD_RLIM64_INFINITY)
>> > krlimits.rlim_cur = RLIM64_INFINITY;
>> > else
>> > krlimits.rlim_cur = rlimits->rlim_cur;
>> > - if (rlimits->rlim_max == OLD_RLIM64_INFINITY)
>> > + if (rlimits->rlim_max >= OLD_RLIM64_INFINITY)
>> > krlimits.rlim_max = RLIM64_INFINITY;
>> > else
>> > krlimits.rlim_max = rlimits->rlim_max;
>>
>> Ditto here we want to do the reverse operations as the
>> rlim64_to_rlim()
>> kernel function.
>
> I don't quite understand where else the change would go. We don't want
> to be touching the generic implementations do we? Or are you saying
> this should actually be going in the kernel and not glibc?
Hi Aurelien, I know this is a bit of a one-off issue, would you mind
elaborating on the comments you left above? Unfortunately it's not
clear enough to me where I should go from here.
@@ -38,11 +38,11 @@ __old_getrlimit64 (enum __rlimit_resource resource,
if (__getrlimit64 (resource, &krlimits) < 0)
return -1;
- if (krlimits.rlim_cur == RLIM64_INFINITY)
+ if (krlimits.rlim_cur >= OLD_RLIM64_INFINITY)
rlimits->rlim_cur = OLD_RLIM64_INFINITY;
else
rlimits->rlim_cur = krlimits.rlim_cur;
- if (krlimits.rlim_max == RLIM64_INFINITY)
+ if (krlimits.rlim_max >= OLD_RLIM64_INFINITY)
rlimits->rlim_max = OLD_RLIM64_INFINITY;
else
rlimits->rlim_max = krlimits.rlim_max;
@@ -35,11 +35,11 @@ __old_setrlimit64 (enum __rlimit_resource resource,
{
struct rlimit64 krlimits;
- if (rlimits->rlim_cur == OLD_RLIM64_INFINITY)
+ if (rlimits->rlim_cur >= OLD_RLIM64_INFINITY)
krlimits.rlim_cur = RLIM64_INFINITY;
else
krlimits.rlim_cur = rlimits->rlim_cur;
- if (rlimits->rlim_max == OLD_RLIM64_INFINITY)
+ if (rlimits->rlim_max >= OLD_RLIM64_INFINITY)
krlimits.rlim_max = RLIM64_INFINITY;
else
krlimits.rlim_max = rlimits->rlim_max;