diff mbox series

Increase judgment on buf.

Message ID 20230519035713.3453563-1-fanpeng@loongson.cn
State Not applicable
Headers
Series Increase judgment on buf. |

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

ticat_fp May 19, 2023, 3:57 a.m. UTC 
  When buf is empty, if it is not checked, the subsequent assignment
operation will trigger a page fault. This is unnecessary.

Signed-off-by: lixing <lixing@loongson.cn>
Signed-off-by: Peng Fan <fanpeng@loongson.cn>
---
 sysdeps/unix/sysv/linux/fstatat64.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Adhemerval Zanella Netto May 19, 2023, 11:48 a.m. UTC | #1 
On 19/05/23 00:57, Peng Fan wrote:
> When buf is empty, if it is not checked, the subsequent assignment
> operation will trigger a page fault. This is unnecessary.
> 
> Signed-off-by: lixing <lixing@loongson.cn>
> Signed-off-by: Peng Fan <fanpeng@loongson.cn>

The stat family is explicitly marked with nonnull for the input struct
stat buffer, and calling with a NULL argument is an UB.

> ---
>  sysdeps/unix/sysv/linux/fstatat64.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/fstatat64.c b/sysdeps/unix/sysv/linux/fstatat64.c
> index 3509d3ca6d..b635a8299a 100644
> --- a/sysdeps/unix/sysv/linux/fstatat64.c
> +++ b/sysdeps/unix/sysv/linux/fstatat64.c
> @@ -52,9 +52,13 @@ fstatat64_time64_statx (int fd, const char *file, struct __stat64_t64 *buf,
>  {
>    /* 32-bit kABI with default 64-bit time_t, e.g. arc, riscv32.   Also
>       64-bit time_t support is done through statx syscall.  */
> -  struct statx tmp;
> +  struct statx tmp, *ptr;
> +  if (buf)
> +	ptr = &tmp;
> +  else
> +	ptr = NULL;
>    int r = INTERNAL_SYSCALL_CALL (statx, fd, file, AT_NO_AUTOMOUNT | flag,
> -				 STATX_BASIC_STATS, &tmp);
> +				 STATX_BASIC_STATS, ptr);
>    if (r != 0)
>      return r;
>
Carlos O'Donell May 19, 2023, 11:55 a.m. UTC | #2 
On 5/19/23 07:48, Adhemerval Zanella Netto via Libc-alpha wrote:
> 
> 
> On 19/05/23 00:57, Peng Fan wrote:
>> When buf is empty, if it is not checked, the subsequent assignment
>> operation will trigger a page fault. This is unnecessary.
>>
>> Signed-off-by: lixing <lixing@loongson.cn>
>> Signed-off-by: Peng Fan <fanpeng@loongson.cn>
> 
> The stat family is explicitly marked with nonnull for the input struct
> stat buffer, and calling with a NULL argument is an UB.

Agreed, and "Style and Conventions"
https://sourceware.org/glibc/wiki/Style_and_Conventions
says:
https://sourceware.org/glibc/wiki/Style_and_Conventions#Bugs_in_the_user_program

We should fail catastrophically and early in the case of user bugs.
The segfault generates a core dump at exactly the right point to debug the UB.

>> ---
>>  sysdeps/unix/sysv/linux/fstatat64.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/sysdeps/unix/sysv/linux/fstatat64.c b/sysdeps/unix/sysv/linux/fstatat64.c
>> index 3509d3ca6d..b635a8299a 100644
>> --- a/sysdeps/unix/sysv/linux/fstatat64.c
>> +++ b/sysdeps/unix/sysv/linux/fstatat64.c
>> @@ -52,9 +52,13 @@ fstatat64_time64_statx (int fd, const char *file, struct __stat64_t64 *buf,
>>  {
>>    /* 32-bit kABI with default 64-bit time_t, e.g. arc, riscv32.   Also
>>       64-bit time_t support is done through statx syscall.  */
>> -  struct statx tmp;
>> +  struct statx tmp, *ptr;
>> +  if (buf)
>> +	ptr = &tmp;
>> +  else
>> +	ptr = NULL;
>>    int r = INTERNAL_SYSCALL_CALL (statx, fd, file, AT_NO_AUTOMOUNT | flag,
>> -				 STATX_BASIC_STATS, &tmp);
>> +				 STATX_BASIC_STATS, ptr);
>>    if (r != 0)
>>      return r;
>>  
>
lixing May 20, 2023, 12:29 a.m. UTC | #3 
在 2023/5/19 下午7:55, Carlos O'Donell via Libc-alpha 写道:
> On 5/19/23 07:48, Adhemerval Zanella Netto via Libc-alpha wrote:
>>
>> On 19/05/23 00:57, Peng Fan wrote:
>>> When buf is empty, if it is not checked, the subsequent assignment
>>> operation will trigger a page fault. This is unnecessary.
>>>
>>> Signed-off-by: lixing <lixing@loongson.cn>
>>> Signed-off-by: Peng Fan <fanpeng@loongson.cn>
>> The stat family is explicitly marked with nonnull for the input struct
>> stat buffer, and calling with a NULL argument is an UB.
> Agreed, and "Style and Conventions"
> https://sourceware.org/glibc/wiki/Style_and_Conventions
> says:
> https://sourceware.org/glibc/wiki/Style_and_Conventions#Bugs_in_the_user_program
>
> We should fail catastrophically and early in the case of user bugs.
> The segfault generates a core dump at exactly the right point to debug the UB.

Yes, LTP fstat03 test pass the buf with NULL. We just want to fail 
earlier during the syscall statx with return value EFAULT if the buf is 
NULL,

but not pass the fault to the struct assignment which trigger SIGSEGV.

>>> ---
>>>   sysdeps/unix/sysv/linux/fstatat64.c | 8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/sysdeps/unix/sysv/linux/fstatat64.c b/sysdeps/unix/sysv/linux/fstatat64.c
>>> index 3509d3ca6d..b635a8299a 100644
>>> --- a/sysdeps/unix/sysv/linux/fstatat64.c
>>> +++ b/sysdeps/unix/sysv/linux/fstatat64.c
>>> @@ -52,9 +52,13 @@ fstatat64_time64_statx (int fd, const char *file, struct __stat64_t64 *buf,
>>>   {
>>>     /* 32-bit kABI with default 64-bit time_t, e.g. arc, riscv32.   Also
>>>        64-bit time_t support is done through statx syscall.  */
>>> -  struct statx tmp;
>>> +  struct statx tmp, *ptr;
>>> +  if (buf)
>>> +	ptr = &tmp;
>>> +  else
>>> +	ptr = NULL;
>>>     int r = INTERNAL_SYSCALL_CALL (statx, fd, file, AT_NO_AUTOMOUNT | flag,
>>> -				 STATX_BASIC_STATS, &tmp);
>>> +				 STATX_BASIC_STATS, ptr);
>>>     if (r != 0)
>>>       return r;
>>>
diff mbox series

Patch

diff --git a/sysdeps/unix/sysv/linux/fstatat64.c b/sysdeps/unix/sysv/linux/fstatat64.c
index 3509d3ca6d..b635a8299a 100644
--- a/sysdeps/unix/sysv/linux/fstatat64.c
+++ b/sysdeps/unix/sysv/linux/fstatat64.c
@@ -52,9 +52,13 @@  fstatat64_time64_statx (int fd, const char *file, struct __stat64_t64 *buf,
 {
   /* 32-bit kABI with default 64-bit time_t, e.g. arc, riscv32.   Also
      64-bit time_t support is done through statx syscall.  */
-  struct statx tmp;
+  struct statx tmp, *ptr;
+  if (buf)
+	ptr = &tmp;
+  else
+	ptr = NULL;
   int r = INTERNAL_SYSCALL_CALL (statx, fd, file, AT_NO_AUTOMOUNT | flag,
-				 STATX_BASIC_STATS, &tmp);
+				 STATX_BASIC_STATS, ptr);
   if (r != 0)
     return r;