setrlimit/getrlimit: Add parameter check to prevent null pointer access
Commit Message
Following sysdeps/mach/hurd/[gs]etrlimit.c.
Add parameter check to prevent null pointer access in setrlimit().
Using Macro Functions RETURN_IF_RLIMIT_EINVAL() to Avoid Duplicate Code Copy.
---
resource/getrlimit64.c | 2 ++
resource/setrlimit64.c | 2 ++
resource/sys/resource.h | 8 ++++++++
sysdeps/mach/hurd/getrlimit.c | 6 +-----
sysdeps/mach/hurd/setrlimit.c | 6 +-----
sysdeps/unix/sysv/linux/alpha/getrlimit64.c | 2 ++
sysdeps/unix/sysv/linux/alpha/setrlimit64.c | 2 ++
sysdeps/unix/sysv/linux/getrlimit64.c | 2 ++
sysdeps/unix/sysv/linux/mips/getrlimit64.c | 2 ++
sysdeps/unix/sysv/linux/mips/setrlimit64.c | 2 ++
sysdeps/unix/sysv/linux/setrlimit.c | 2 ++
11 files changed, 26 insertions(+), 10 deletions(-)
Comments
On 30/12/2020 08:41, Xiaoming Ni wrote:
> Following sysdeps/mach/hurd/[gs]etrlimit.c.
> Add parameter check to prevent null pointer access in setrlimit().
> Using Macro Functions RETURN_IF_RLIMIT_EINVAL() to Avoid Duplicate Code Copy.
POSIX [1] does not define that EINVAL should be returned for invalid
argument, only for an invalid resource or if limit specified cannot
be lowered.
LTP used to check for a *Linux* specific semantic where it returned
EFAULT for such cases; but it was a wrong assumption since getrlimit
could be reimplemented through another syscall (such as currently
on glibc it calls prlimit).
I think we should mark the argument nonull instead and remove this
check from Hurd (it will need to check againt RLIMIT_NLIMITS
since it access a internal array).
[1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/getrlimit.html
> ---
> resource/getrlimit64.c | 2 ++
> resource/setrlimit64.c | 2 ++
> resource/sys/resource.h | 8 ++++++++
> sysdeps/mach/hurd/getrlimit.c | 6 +-----
> sysdeps/mach/hurd/setrlimit.c | 6 +-----
> sysdeps/unix/sysv/linux/alpha/getrlimit64.c | 2 ++
> sysdeps/unix/sysv/linux/alpha/setrlimit64.c | 2 ++
> sysdeps/unix/sysv/linux/getrlimit64.c | 2 ++
> sysdeps/unix/sysv/linux/mips/getrlimit64.c | 2 ++
> sysdeps/unix/sysv/linux/mips/setrlimit64.c | 2 ++
> sysdeps/unix/sysv/linux/setrlimit.c | 2 ++
> 11 files changed, 26 insertions(+), 10 deletions(-)
>
> diff --git a/resource/getrlimit64.c b/resource/getrlimit64.c
> index 41b6fa01d2..84059a5db8 100644
> --- a/resource/getrlimit64.c
> +++ b/resource/getrlimit64.c
> @@ -26,6 +26,8 @@ __getrlimit64 (enum __rlimit_resource resource, struct rlimit64 *rlimits)
> {
> struct rlimit rlimits32;
>
> + RETURN_IF_RLIMIT_EINVAL(resource, rlimits);
> +
> if (__getrlimit (resource, &rlimits32) < 0)
> return -1;
>
> diff --git a/resource/setrlimit64.c b/resource/setrlimit64.c
> index 0411e3ea19..687df56cb3 100644
> --- a/resource/setrlimit64.c
> +++ b/resource/setrlimit64.c
> @@ -27,6 +27,8 @@ setrlimit64 (enum __rlimit_resource resource, const struct rlimit64 *rlimits)
> {
> struct rlimit rlimits32;
>
> + RETURN_IF_RLIMIT_EINVAL(resource, rlimits);
> +
> if (rlimits->rlim_cur >= RLIM_INFINITY)
> rlimits32.rlim_cur = RLIM_INFINITY;
> else
> diff --git a/resource/sys/resource.h b/resource/sys/resource.h
> index 4edafb50d5..a98b7c8b4e 100644
> --- a/resource/sys/resource.h
> +++ b/resource/sys/resource.h
> @@ -82,6 +82,14 @@ extern int setrlimit64 (__rlimit_resource_t __resource,
> const struct rlimit64 *__rlimits) __THROW;
> #endif
>
> +#define RETURN_IF_RLIMIT_EINVAL(resource, rlimits) do { \
> + if ((rlimits) == NULL || (unsigned int) (resource) >= RLIMIT_NLIMITS) \
> + { \
> + errno = EINVAL; \
> + return -1; \
> + } \
> +} while(0)
> +
> /* Return resource usage information on process indicated by WHO
> and put it in *USAGE. Returns 0 for success, -1 for failure. */
> extern int getrusage (__rusage_who_t __who, struct rusage *__usage) __THROW;
> diff --git a/sysdeps/mach/hurd/getrlimit.c b/sysdeps/mach/hurd/getrlimit.c
> index 32d37c185d..904a9b7b9b 100644
> --- a/sysdeps/mach/hurd/getrlimit.c
> +++ b/sysdeps/mach/hurd/getrlimit.c
> @@ -27,11 +27,7 @@ __getrlimit (enum __rlimit_resource resource, struct rlimit *rlimits)
> {
> struct rlimit lim;
>
> - if (rlimits == NULL || (unsigned int) resource >= RLIMIT_NLIMITS)
> - {
> - errno = EINVAL;
> - return -1;
> - }
> + RETURN_IF_RLIMIT_EINVAL(resource, rlimits);
>
> HURD_CRITICAL_BEGIN;
> __mutex_lock (&_hurd_rlimit_lock);
> diff --git a/sysdeps/mach/hurd/setrlimit.c b/sysdeps/mach/hurd/setrlimit.c
> index e0f80bbb9c..a1b5615b93 100644
> --- a/sysdeps/mach/hurd/setrlimit.c
> +++ b/sysdeps/mach/hurd/setrlimit.c
> @@ -29,11 +29,7 @@ __setrlimit (enum __rlimit_resource resource, const struct rlimit *rlimits)
> {
> struct rlimit lim;
>
> - if (rlimits == NULL || (unsigned int) resource >= RLIMIT_NLIMITS)
> - {
> - errno = EINVAL;
> - return -1;
> - }
> + RETURN_IF_RLIMIT_EINVAL(resource, rlimits);
>
> lim = *rlimits;
>
> diff --git a/sysdeps/unix/sysv/linux/alpha/getrlimit64.c b/sysdeps/unix/sysv/linux/alpha/getrlimit64.c
> index 96655ff77e..017a7c18d4 100644
> --- a/sysdeps/unix/sysv/linux/alpha/getrlimit64.c
> +++ b/sysdeps/unix/sysv/linux/alpha/getrlimit64.c
> @@ -35,6 +35,8 @@ __old_getrlimit64 (enum __rlimit_resource resource,
> {
> struct rlimit64 krlimits;
>
> + RETURN_IF_RLIMIT_EINVAL(resource, rlimits);
> +
> if (__getrlimit64 (resource, &krlimits) < 0)
> return -1;
>
> diff --git a/sysdeps/unix/sysv/linux/alpha/setrlimit64.c b/sysdeps/unix/sysv/linux/alpha/setrlimit64.c
> index ae77b4127c..8c5ac6c76e 100644
> --- a/sysdeps/unix/sysv/linux/alpha/setrlimit64.c
> +++ b/sysdeps/unix/sysv/linux/alpha/setrlimit64.c
> @@ -35,6 +35,8 @@ __old_setrlimit64 (enum __rlimit_resource resource,
> {
> struct rlimit64 krlimits;
>
> + RETURN_IF_RLIMIT_EINVAL(resource, rlimits);
> +
> if (rlimits->rlim_cur == OLD_RLIM64_INFINITY)
> krlimits.rlim_cur = RLIM64_INFINITY;
> else
> diff --git a/sysdeps/unix/sysv/linux/getrlimit64.c b/sysdeps/unix/sysv/linux/getrlimit64.c
> index e06ffd1a16..e75d06a402 100644
> --- a/sysdeps/unix/sysv/linux/getrlimit64.c
> +++ b/sysdeps/unix/sysv/linux/getrlimit64.c
> @@ -66,6 +66,8 @@ __old_getrlimit64 (enum __rlimit_resource resource, struct rlimit64 *rlimits)
> {
> struct rlimit rlimits32;
>
> + RETURN_IF_RLIMIT_EINVAL(resource, rlimits);
> +
> if (__new_getrlimit (resource, &rlimits32) < 0)
> return -1;
>
> diff --git a/sysdeps/unix/sysv/linux/mips/getrlimit64.c b/sysdeps/unix/sysv/linux/mips/getrlimit64.c
> index f42100f085..94008444a9 100644
> --- a/sysdeps/unix/sysv/linux/mips/getrlimit64.c
> +++ b/sysdeps/unix/sysv/linux/mips/getrlimit64.c
> @@ -45,6 +45,8 @@ __old_getrlimit64 (enum __rlimit_resource resource,
> {
> struct rlimit64 krlimits;
>
> + RETURN_IF_RLIMIT_EINVAL(resource, rlimits);
> +
> if (__getrlimit64 (resource, &krlimits) < 0)
> return -1;
>
> diff --git a/sysdeps/unix/sysv/linux/mips/setrlimit64.c b/sysdeps/unix/sysv/linux/mips/setrlimit64.c
> index 36f5d85378..10ae33bf37 100644
> --- a/sysdeps/unix/sysv/linux/mips/setrlimit64.c
> +++ b/sysdeps/unix/sysv/linux/mips/setrlimit64.c
> @@ -44,6 +44,8 @@ __old_setrlimit64 (enum __rlimit_resource resource,
> {
> struct rlimit64 krlimits;
>
> + RETURN_IF_RLIMIT_EINVAL(resource, rlimits);
> +
> if (rlimits->rlim_cur == OLD_RLIM64_INFINITY)
> krlimits.rlim_cur = RLIM64_INFINITY;
> else
> diff --git a/sysdeps/unix/sysv/linux/setrlimit.c b/sysdeps/unix/sysv/linux/setrlimit.c
> index 6648fad5c0..98659c4ced 100644
> --- a/sysdeps/unix/sysv/linux/setrlimit.c
> +++ b/sysdeps/unix/sysv/linux/setrlimit.c
> @@ -35,6 +35,8 @@ __setrlimit (enum __rlimit_resource resource, const struct rlimit *rlim)
> {
> struct rlimit64 rlim64;
>
> + RETURN_IF_RLIMIT_EINVAL(resource, rlim);
> +
> if (rlim->rlim_cur == RLIM_INFINITY)
> rlim64.rlim_cur = RLIM64_INFINITY;
> else
>
On 2020/12/30 20:43, Adhemerval Zanella wrote:
>
>
> On 30/12/2020 08:41, Xiaoming Ni wrote:
>> Following sysdeps/mach/hurd/[gs]etrlimit.c.
>> Add parameter check to prevent null pointer access in setrlimit().
>> Using Macro Functions RETURN_IF_RLIMIT_EINVAL() to Avoid Duplicate Code Copy.
>
> POSIX [1] does not define that EINVAL should be returned for invalid
> argument, only for an invalid resource or if limit specified cannot
> be lowered.
>
> LTP used to check for a *Linux* specific semantic where it returned
> EFAULT for such cases; but it was a wrong assumption since getrlimit
> could be reimplemented through another syscall (such as currently
> on glibc it calls prlimit).
>
> I think we should mark the argument nonull instead and remove this
> check from Hurd (it will need to check againt RLIMIT_NLIMITS
> since it access a internal array).
>
> [1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/getrlimit.html
>
Is that so?
diff --git a/resource/setrlimit64.c b/resource/setrlimit64.c
index 0411e3ea19..c73ce59139 100644
--- a/resource/setrlimit64.c
+++ b/resource/setrlimit64.c
@@ -27,6 +27,8 @@ setrlimit64 (enum __rlimit_resource resource, const
struct rlimit64 *rlimits)
{
struct rlimit rlimits32;
+ RETURN_IF_RLIMITS_IS_NULL(rlimits);
+
if (rlimits->rlim_cur >= RLIM_INFINITY)
rlimits32.rlim_cur = RLIM_INFINITY;
else
diff --git a/resource/sys/resource.h b/resource/sys/resource.h
index 4edafb50d5..c95ef35bfc 100644
--- a/resource/sys/resource.h
+++ b/resource/sys/resource.h
@@ -82,6 +82,14 @@ extern int setrlimit64 (__rlimit_resource_t __resource,
const struct rlimit64 *__rlimits) __THROW;
#endif
+#define RETURN_IF_RLIMITS_IS_NULL(rlimits) do { \
+ if ((rlimits) == NULL) \
+ { \
+ errno = EFAULT; \
+ return -1; \
+ } \
+} while(0)
+
/* Return resource usage information on process indicated by WHO
and put it in *USAGE. Returns 0 for success, -1 for failure. */
extern int getrusage (__rusage_who_t __who, struct rusage *__usage)
__THROW;
diff --git a/sysdeps/mach/hurd/getrlimit.c b/sysdeps/mach/hurd/getrlimit.c
index 32d37c185d..3ffcf80fc3 100644
--- a/sysdeps/mach/hurd/getrlimit.c
+++ b/sysdeps/mach/hurd/getrlimit.c
@@ -27,7 +27,8 @@ __getrlimit (enum __rlimit_resource resource, struct
rlimit *rlimits)
{
struct rlimit lim;
- if (rlimits == NULL || (unsigned int) resource >= RLIMIT_NLIMITS)
+ RETURN_IF_RLIMITS_IS_NULL(rlimits);
+ if ((unsigned int) (resource) >= RLIMIT_NLIMITS)
{
errno = EINVAL;
return -1;
On 2020/12/31 10:44, Xiaoming Ni wrote:
> On 2020/12/30 20:43, Adhemerval Zanella wrote:
>>
>>
>> On 30/12/2020 08:41, Xiaoming Ni wrote:
>>> Following sysdeps/mach/hurd/[gs]etrlimit.c.
>>> Add parameter check to prevent null pointer access in setrlimit().
>>> Using Macro Functions RETURN_IF_RLIMIT_EINVAL() to Avoid Duplicate
>>> Code Copy.
>>
>> POSIX [1] does not define that EINVAL should be returned for invalid
>> argument, only for an invalid resource or if limit specified cannot
>> be lowered.
>>
>> LTP used to check for a *Linux* specific semantic where it returned
>> EFAULT for such cases; but it was a wrong assumption since getrlimit
>> could be reimplemented through another syscall (such as currently
>> on glibc it calls prlimit).
>>
>> I think we should mark the argument nonull instead and remove this
>> check from Hurd (it will need to check againt RLIMIT_NLIMITS
>> since it access a internal array).
>>
>> [1]
>> https://pubs.opengroup.org/onlinepubs/9699919799/functions/getrlimit.html
>>
>
> Is that so?
>
> diff --git a/resource/setrlimit64.c b/resource/setrlimit64.c
> index 0411e3ea19..c73ce59139 100644
> --- a/resource/setrlimit64.c
> +++ b/resource/setrlimit64.c
> @@ -27,6 +27,8 @@ setrlimit64 (enum __rlimit_resource resource, const
> struct rlimit64 *rlimits)
> {
> struct rlimit rlimits32;
>
> + RETURN_IF_RLIMITS_IS_NULL(rlimits);
> +
> if (rlimits->rlim_cur >= RLIM_INFINITY)
> rlimits32.rlim_cur = RLIM_INFINITY;
> else
> diff --git a/resource/sys/resource.h b/resource/sys/resource.h
> index 4edafb50d5..c95ef35bfc 100644
> --- a/resource/sys/resource.h
> +++ b/resource/sys/resource.h
> @@ -82,6 +82,14 @@ extern int setrlimit64 (__rlimit_resource_t __resource,
> const struct rlimit64 *__rlimits) __THROW;
> #endif
>
> +#define RETURN_IF_RLIMITS_IS_NULL(rlimits) do { \
> + if ((rlimits) == NULL) \
> + { \
> + errno = EFAULT; \
> + return -1; \
> + } \
> +} while(0)
> +
> /* Return resource usage information on process indicated by WHO
> and put it in *USAGE. Returns 0 for success, -1 for failure. */
> extern int getrusage (__rusage_who_t __who, struct rusage *__usage)
> __THROW;
> diff --git a/sysdeps/mach/hurd/getrlimit.c b/sysdeps/mach/hurd/getrlimit.c
> index 32d37c185d..3ffcf80fc3 100644
> --- a/sysdeps/mach/hurd/getrlimit.c
> +++ b/sysdeps/mach/hurd/getrlimit.c
> @@ -27,7 +27,8 @@ __getrlimit (enum __rlimit_resource resource, struct
> rlimit *rlimits)
> {
> struct rlimit lim;
>
> - if (rlimits == NULL || (unsigned int) resource >= RLIMIT_NLIMITS)
> + RETURN_IF_RLIMITS_IS_NULL(rlimits);
> + if ((unsigned int) (resource) >= RLIMIT_NLIMITS)
> {
> errno = EINVAL;
> return -1;
>
>
>
or this?
--- a/resource/sys/resource.h
+++ b/resource/sys/resource.h
@@ -67,7 +67,8 @@ extern int getrlimit64 (__rlimit_resource_t __resource,
Return 0 if successful, -1 if not (and sets errno). */
#ifndef __USE_FILE_OFFSET64
extern int setrlimit (__rlimit_resource_t __resource,
- const struct rlimit *__rlimits) __THROW;
+ const struct rlimit *__rlimits)
+ __THROW __nonnull ((2));;
#else
On 31/12/2020 01:01, Xiaoming Ni wrote:
> On 2020/12/31 10:44, Xiaoming Ni wrote:
>> On 2020/12/30 20:43, Adhemerval Zanella wrote:
>>>
>>>
>>> On 30/12/2020 08:41, Xiaoming Ni wrote:
>>>> Following sysdeps/mach/hurd/[gs]etrlimit.c.
>>>> Add parameter check to prevent null pointer access in setrlimit().
>>>> Using Macro Functions RETURN_IF_RLIMIT_EINVAL() to Avoid Duplicate Code Copy.
>>>
>>> POSIX [1] does not define that EINVAL should be returned for invalid
>>> argument, only for an invalid resource or if limit specified cannot
>>> be lowered.
>>>
>>> LTP used to check for a *Linux* specific semantic where it returned
>>> EFAULT for such cases; but it was a wrong assumption since getrlimit
>>> could be reimplemented through another syscall (such as currently
>>> on glibc it calls prlimit).
>>>
>>> I think we should mark the argument nonull instead and remove this
>>> check from Hurd (it will need to check againt RLIMIT_NLIMITS
>>> since it access a internal array).
>>>
>>> [1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/getrlimit.html
>>>
>>
>> Is that so?
>>
>> diff --git a/resource/setrlimit64.c b/resource/setrlimit64.c
>> index 0411e3ea19..c73ce59139 100644
>> --- a/resource/setrlimit64.c
>> +++ b/resource/setrlimit64.c
>> @@ -27,6 +27,8 @@ setrlimit64 (enum __rlimit_resource resource, const struct rlimit64 *rlimits)
>> {
>> struct rlimit rlimits32;
>>
>> + RETURN_IF_RLIMITS_IS_NULL(rlimits);
>> +
>> if (rlimits->rlim_cur >= RLIM_INFINITY)
>> rlimits32.rlim_cur = RLIM_INFINITY;
>> else
>> diff --git a/resource/sys/resource.h b/resource/sys/resource.h
>> index 4edafb50d5..c95ef35bfc 100644
>> --- a/resource/sys/resource.h
>> +++ b/resource/sys/resource.h
>> @@ -82,6 +82,14 @@ extern int setrlimit64 (__rlimit_resource_t __resource,
>> const struct rlimit64 *__rlimits) __THROW;
>> #endif
>>
>> +#define RETURN_IF_RLIMITS_IS_NULL(rlimits) do { \
>> + if ((rlimits) == NULL) \
>> + { \
>> + errno = EFAULT; \
>> + return -1; \
>> + } \
>> +} while(0)
>> +
>> /* Return resource usage information on process indicated by WHO
>> and put it in *USAGE. Returns 0 for success, -1 for failure. */
>> extern int getrusage (__rusage_who_t __who, struct rusage *__usage) __THROW;
>> diff --git a/sysdeps/mach/hurd/getrlimit.c b/sysdeps/mach/hurd/getrlimit.c
>> index 32d37c185d..3ffcf80fc3 100644
>> --- a/sysdeps/mach/hurd/getrlimit.c
>> +++ b/sysdeps/mach/hurd/getrlimit.c
>> @@ -27,7 +27,8 @@ __getrlimit (enum __rlimit_resource resource, struct rlimit *rlimits)
>> {
>> struct rlimit lim;
>>
>> - if (rlimits == NULL || (unsigned int) resource >= RLIMIT_NLIMITS)
>> + RETURN_IF_RLIMITS_IS_NULL(rlimits);
>> + if ((unsigned int) (resource) >= RLIMIT_NLIMITS)
>> {
>> errno = EINVAL;
>> return -1;
>>
>>
>>
>
> or this?
>
> --- a/resource/sys/resource.h
> +++ b/resource/sys/resource.h
> @@ -67,7 +67,8 @@ extern int getrlimit64 (__rlimit_resource_t __resource,
> Return 0 if successful, -1 if not (and sets errno). */
> #ifndef __USE_FILE_OFFSET64
> extern int setrlimit (__rlimit_resource_t __resource,
> - const struct rlimit *__rlimits) __THROW;
> + const struct rlimit *__rlimits)
> + __THROW __nonnull ((2));;
> #else
>
>
Yes, you might also add the attribute on getrlimit and also on prlimit.
On 2020/12/31 18:52, Adhemerval Zanella wrote:
>
>
> On 31/12/2020 01:01, Xiaoming Ni wrote:
>> On 2020/12/31 10:44, Xiaoming Ni wrote:
>>> On 2020/12/30 20:43, Adhemerval Zanella wrote:
>>>>
>>>>
>>>> On 30/12/2020 08:41, Xiaoming Ni wrote:
>>>>> Following sysdeps/mach/hurd/[gs]etrlimit.c.
>>>>> Add parameter check to prevent null pointer access in setrlimit().
>>>>> Using Macro Functions RETURN_IF_RLIMIT_EINVAL() to Avoid Duplicate Code Copy.
>>>>
>>>> POSIX [1] does not define that EINVAL should be returned for invalid
>>>> argument, only for an invalid resource or if limit specified cannot
>>>> be lowered.
>>>>
>>>> LTP used to check for a *Linux* specific semantic where it returned
>>>> EFAULT for such cases; but it was a wrong assumption since getrlimit
>>>> could be reimplemented through another syscall (such as currently
>>>> on glibc it calls prlimit).
>>>>
>>>> I think we should mark the argument nonull instead and remove this
>>>> check from Hurd (it will need to check againt RLIMIT_NLIMITS
>>>> since it access a internal array).
>>>>
>>>> [1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/getrlimit.html
>>>>
>>>
>>> Is that so?
>>>
>>> diff --git a/resource/setrlimit64.c b/resource/setrlimit64.c
>>> index 0411e3ea19..c73ce59139 100644
>>> --- a/resource/setrlimit64.c
>>> +++ b/resource/setrlimit64.c
>>> @@ -27,6 +27,8 @@ setrlimit64 (enum __rlimit_resource resource, const struct rlimit64 *rlimits)
>>> {
>>> struct rlimit rlimits32;
>>>
>>> + RETURN_IF_RLIMITS_IS_NULL(rlimits);
>>> +
>>> if (rlimits->rlim_cur >= RLIM_INFINITY)
>>> rlimits32.rlim_cur = RLIM_INFINITY;
>>> else
>>> diff --git a/resource/sys/resource.h b/resource/sys/resource.h
>>> index 4edafb50d5..c95ef35bfc 100644
>>> --- a/resource/sys/resource.h
>>> +++ b/resource/sys/resource.h
>>> @@ -82,6 +82,14 @@ extern int setrlimit64 (__rlimit_resource_t __resource,
>>> const struct rlimit64 *__rlimits) __THROW;
>>> #endif
>>>
>>> +#define RETURN_IF_RLIMITS_IS_NULL(rlimits) do { \
>>> + if ((rlimits) == NULL) \
>>> + { \
>>> + errno = EFAULT; \
>>> + return -1; \
>>> + } \
>>> +} while(0)
>>> +
>>> /* Return resource usage information on process indicated by WHO
>>> and put it in *USAGE. Returns 0 for success, -1 for failure. */
>>> extern int getrusage (__rusage_who_t __who, struct rusage *__usage) __THROW;
>>> diff --git a/sysdeps/mach/hurd/getrlimit.c b/sysdeps/mach/hurd/getrlimit.c
>>> index 32d37c185d..3ffcf80fc3 100644
>>> --- a/sysdeps/mach/hurd/getrlimit.c
>>> +++ b/sysdeps/mach/hurd/getrlimit.c
>>> @@ -27,7 +27,8 @@ __getrlimit (enum __rlimit_resource resource, struct rlimit *rlimits)
>>> {
>>> struct rlimit lim;
>>>
>>> - if (rlimits == NULL || (unsigned int) resource >= RLIMIT_NLIMITS)
>>> + RETURN_IF_RLIMITS_IS_NULL(rlimits);
>>> + if ((unsigned int) (resource) >= RLIMIT_NLIMITS)
>>> {
>>> errno = EINVAL;
>>> return -1;
>>>
>>>
>>>
>>
>> or this?
>>
>> --- a/resource/sys/resource.h
>> +++ b/resource/sys/resource.h
>> @@ -67,7 +67,8 @@ extern int getrlimit64 (__rlimit_resource_t __resource,
>> Return 0 if successful, -1 if not (and sets errno). */
>> #ifndef __USE_FILE_OFFSET64
>> extern int setrlimit (__rlimit_resource_t __resource,
>> - const struct rlimit *__rlimits) __THROW;
>> + const struct rlimit *__rlimits)
>> + __THROW __nonnull ((2));;
>> #else
>>
>>
>
> Yes, you might also add the attribute on getrlimit and also on prlimit.
> .
>
__nonnull((2)) can only check null pointers in the compilation phase,
As an external interface, the getrlimit()/setrlimit()/prlimit() needs to
check the null pointer during the running of the code to prevent the
system from crashing.
so, Is it better to use both RETURN_IF_RLIMITS_IS_NULL() and __nonnull()?
Thanks
Xiaoming NI
On Sat, Jan 2, 2021 at 10:20 PM Xiaoming Ni wrote:
> __nonnull((2)) can only check null pointers in the compilation phase,
As an external interface, the getrlimit()/setrlimit()/prlimit() needs to
> check the null pointer during the running of the code to prevent the
> system from crashing.
This is a common misconception. Most C library functions *should* crash the
process when they receive invalid pointer arguments.
To implement getrlimit on top of prlimit, you need to do something like
this:
memset (rlim, 0, sizeof *rlim);
return prlimit (0, rsrc, 0, rlim);
so that the program crashes instead of doing nothing when rlim is null.
zw
>
On 2021/1/3 12:22, Zack Weinberg wrote:
> This is a common misconception. Most C library functions *should* crash
> the process when they receive invalid pointer arguments.
>
> To implement getrlimit on top of prlimit, you need to do something like
> this:
>
> memset (rlim, 0, sizeof *rlim);
> return prlimit (0, rsrc, 0, rlim);
>
> so that the program crashes instead of doing nothing when rlim is null.
>
> zw
While adding a layer of parameter validation and returning an error
message may cause a slight performance loss, would a program crash due
to lack of validation be considered a more serious failure? For example,
a denial of service:
https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=NULL+pointer
Currently, a large number of C library functions do not strictly check
pointer parameters. Is it because the historical inventory is hard to
fix, or is it deliberately designed?
Thanks
Xiaoming Ni.
On Sun, Jan 3, 2021 at 2:42 AM Xiaoming Ni <nixiaoming@huawei.com> wrote:
> On 2021/1/3 12:22, Zack Weinberg wrote:
> > This is a common misconception. Most C library functions *should* crash
> > the process when they receive invalid pointer arguments.
>
> While adding a layer of parameter validation and returning an error
> message may cause a slight performance loss, would a program crash due
> to lack of validation be considered a more serious failure?
...
> Currently, a large number of C library functions do not strictly check
> pointer parameters. Is it because the historical inventory is hard to
> fix, or is it deliberately designed?
Not "strictly checking" pointers is deliberate, and it is not for
performance reasons, nor is it because of a lack of attention to
security. It is *correct* for a C library function to react to an
invalid pointer argument by crashing the process; it is *incorrect* to
return an error.
This is for two reasons. First, most invalid pointers cannot be
detected by inspecting their values. If someone calls getrlimit(...,
(struct rlimit *) 0x01020304)), the only way for getrlimit to know
that it has been given an invalid pointer is to attempt to write
through it and ... crash the process. It's cleaner to treat invalid
pointers that *can* be detected (like NULL) the same way as those that
can't; error recovery code only has to handle one case. Second,
passing an invalid pointer (including NULL, in this case) to a C
library function is a *bug*, and bugs of this type are easier to fix
if the program crashes as promptly as possible. Probably nobody is
expecting getrlimit to fail with EINVAL or EFAULT, so, your proposed
patch would make the program keep going for some time after the
initial invalid operation, getting further off track and, most likely,
crashing later, in code a long way away from where the root cause of
the bug was.
You are right that crashing can cause denial of service, but the
proper way to handle that is with service management code that logs
the failure and restarts the crashed server. Allowing a service
process to continue running after it's hit a memory corruption bug
leads to *exploitable* vulnerabilities, which are much worse than
denial of service.
Some C library functions do react to a NULL pointer in a predictable
way that isn't crashing. For instance, free(0) is a no-op, and
sigaction(SIGxxx, &sa, 0) sets a new signal handler and doesn't tell
the caller what the old handler was. When a function does this, NULL
is a *valid* argument to that function, and the behavior is clearly
documented and not considered an error. It's useful for prlimit to be
one of these, since that lets it get *or* set rlimits, or both at the
same time. It's not useful for getrlimit to be one of these.
zw
On 2021/1/4 2:02, Zack Weinberg wrote:
> Not "strictly checking" pointers is deliberate, and it is not for
> performance reasons, nor is it because of a lack of attention to
> security. It is*correct* for a C library function to react to an
> invalid pointer argument by crashing the process; it is*incorrect* to
> return an error.
>
> This is for two reasons. First, most invalid pointers cannot be
> detected by inspecting their values. If someone calls getrlimit(...,
> (struct rlimit *) 0x01020304)), the only way for getrlimit to know
> that it has been given an invalid pointer is to attempt to write
> through it and ... crash the process. It's cleaner to treat invalid
> pointers that*can* be detected (like NULL) the same way as those that
> can't; error recovery code only has to handle one case. Second,
> passing an invalid pointer (including NULL, in this case) to a C
> library function is a*bug*, and bugs of this type are easier to fix
> if the program crashes as promptly as possible. Probably nobody is
> expecting getrlimit to fail with EINVAL or EFAULT, so, your proposed
> patch would make the program keep going for some time after the
> initial invalid operation, getting further off track and, most likely,
> crashing later, in code a long way away from where the root cause of
> the bug was.
>
> You are right that crashing can cause denial of service, but the
> proper way to handle that is with service management code that logs
> the failure and restarts the crashed server. Allowing a service
> process to continue running after it's hit a memory corruption bug
> leads to*exploitable* vulnerabilities, which are much worse than
> denial of service.
>
> Some C library functions do react to a NULL pointer in a predictable
> way that isn't crashing. For instance, free(0) is a no-op, and
> sigaction(SIGxxx, &sa, 0) sets a new signal handler and doesn't tell
> the caller what the old handler was. When a function does this, NULL
> is a*valid* argument to that function, and the behavior is clearly
> documented and not considered an error. It's useful for prlimit to be
> one of these, since that lets it get*or* set rlimits, or both at the
> same time. It's not useful for getrlimit to be one of these.
>
> zw
Thank you for your detailed explanation.
I will send v2 patches later based on review comments
Thanks
Xiaoming Ni
@@ -26,6 +26,8 @@ __getrlimit64 (enum __rlimit_resource resource, struct rlimit64 *rlimits)
{
struct rlimit rlimits32;
+ RETURN_IF_RLIMIT_EINVAL(resource, rlimits);
+
if (__getrlimit (resource, &rlimits32) < 0)
return -1;
@@ -27,6 +27,8 @@ setrlimit64 (enum __rlimit_resource resource, const struct rlimit64 *rlimits)
{
struct rlimit rlimits32;
+ RETURN_IF_RLIMIT_EINVAL(resource, rlimits);
+
if (rlimits->rlim_cur >= RLIM_INFINITY)
rlimits32.rlim_cur = RLIM_INFINITY;
else
@@ -82,6 +82,14 @@ extern int setrlimit64 (__rlimit_resource_t __resource,
const struct rlimit64 *__rlimits) __THROW;
#endif
+#define RETURN_IF_RLIMIT_EINVAL(resource, rlimits) do { \
+ if ((rlimits) == NULL || (unsigned int) (resource) >= RLIMIT_NLIMITS) \
+ { \
+ errno = EINVAL; \
+ return -1; \
+ } \
+} while(0)
+
/* Return resource usage information on process indicated by WHO
and put it in *USAGE. Returns 0 for success, -1 for failure. */
extern int getrusage (__rusage_who_t __who, struct rusage *__usage) __THROW;
@@ -27,11 +27,7 @@ __getrlimit (enum __rlimit_resource resource, struct rlimit *rlimits)
{
struct rlimit lim;
- if (rlimits == NULL || (unsigned int) resource >= RLIMIT_NLIMITS)
- {
- errno = EINVAL;
- return -1;
- }
+ RETURN_IF_RLIMIT_EINVAL(resource, rlimits);
HURD_CRITICAL_BEGIN;
__mutex_lock (&_hurd_rlimit_lock);
@@ -29,11 +29,7 @@ __setrlimit (enum __rlimit_resource resource, const struct rlimit *rlimits)
{
struct rlimit lim;
- if (rlimits == NULL || (unsigned int) resource >= RLIMIT_NLIMITS)
- {
- errno = EINVAL;
- return -1;
- }
+ RETURN_IF_RLIMIT_EINVAL(resource, rlimits);
lim = *rlimits;
@@ -35,6 +35,8 @@ __old_getrlimit64 (enum __rlimit_resource resource,
{
struct rlimit64 krlimits;
+ RETURN_IF_RLIMIT_EINVAL(resource, rlimits);
+
if (__getrlimit64 (resource, &krlimits) < 0)
return -1;
@@ -35,6 +35,8 @@ __old_setrlimit64 (enum __rlimit_resource resource,
{
struct rlimit64 krlimits;
+ RETURN_IF_RLIMIT_EINVAL(resource, rlimits);
+
if (rlimits->rlim_cur == OLD_RLIM64_INFINITY)
krlimits.rlim_cur = RLIM64_INFINITY;
else
@@ -66,6 +66,8 @@ __old_getrlimit64 (enum __rlimit_resource resource, struct rlimit64 *rlimits)
{
struct rlimit rlimits32;
+ RETURN_IF_RLIMIT_EINVAL(resource, rlimits);
+
if (__new_getrlimit (resource, &rlimits32) < 0)
return -1;
@@ -45,6 +45,8 @@ __old_getrlimit64 (enum __rlimit_resource resource,
{
struct rlimit64 krlimits;
+ RETURN_IF_RLIMIT_EINVAL(resource, rlimits);
+
if (__getrlimit64 (resource, &krlimits) < 0)
return -1;
@@ -44,6 +44,8 @@ __old_setrlimit64 (enum __rlimit_resource resource,
{
struct rlimit64 krlimits;
+ RETURN_IF_RLIMIT_EINVAL(resource, rlimits);
+
if (rlimits->rlim_cur == OLD_RLIM64_INFINITY)
krlimits.rlim_cur = RLIM64_INFINITY;
else
@@ -35,6 +35,8 @@ __setrlimit (enum __rlimit_resource resource, const struct rlimit *rlim)
{
struct rlimit64 rlim64;
+ RETURN_IF_RLIMIT_EINVAL(resource, rlim);
+
if (rlim->rlim_cur == RLIM_INFINITY)
rlim64.rlim_cur = RLIM64_INFINITY;
else