diff mbox series

arc4random: Fix incorrect usage of TEMP_FAILURE_RETRY

Message ID 20240104005411.31500-2-xry111@xry111.site
State Superseded
Headers
Series arc4random: Fix incorrect usage of TEMP_FAILURE_RETRY |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Testing passed

Commit Message

Xi Ruoyao Jan. 4, 2024, 12:54 a.m. UTC 
  The _nocancel functions returns errors as negative values, i. e. -EINTR
for interrupted system call.  But TEMP_FAILURE_RETRY expects the errors
as errno, thus using TEMP_FAILURE_RETRY is incorrect here.

Add a customized TEMP_FAILURE_RETRY_NEG macro and use it instead of
TEMP_FAILURE_RETRY to fix the issue.

Signed-off-by: Xi Ruoyao <xry111@xry111.site>
---
 stdlib/arc4random.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

Comments

Florian Weimer Jan. 4, 2024, 9:51 a.m. UTC | #1 
* Xi Ruoyao:

> The _nocancel functions returns errors as negative values, i. e. -EINTR
> for interrupted system call.  But TEMP_FAILURE_RETRY expects the errors
> as errno, thus using TEMP_FAILURE_RETRY is incorrect here.
>
> Add a customized TEMP_FAILURE_RETRY_NEG macro and use it instead of
> TEMP_FAILURE_RETRY to fix the issue.

Or we could change __getrandom_nocancel to use INLINE_SYSCALL_CALL
instead of INTERNAL_SYCALL_CALL.

Thanks,
Florian
Adhemerval Zanella Netto Jan. 4, 2024, 12:13 p.m. UTC | #2 
On 04/01/24 06:51, Florian Weimer wrote:
> * Xi Ruoyao:
> 
>> The _nocancel functions returns errors as negative values, i. e. -EINTR
>> for interrupted system call.  But TEMP_FAILURE_RETRY expects the errors
>> as errno, thus using TEMP_FAILURE_RETRY is incorrect here.
>>
>> Add a customized TEMP_FAILURE_RETRY_NEG macro and use it instead of
>> TEMP_FAILURE_RETRY to fix the issue.
> 
> Or we could change __getrandom_nocancel to use INLINE_SYSCALL_CALL
> instead of INTERNAL_SYCALL_CALL.

It follows what other _nocancel functions that return a value do.  The
__getrandom_nocancel is also used on malloc initialization, but I do
not think touching errno should be an issue.
Florian Weimer Jan. 4, 2024, 12:16 p.m. UTC | #3 
* Adhemerval Zanella Netto:

> On 04/01/24 06:51, Florian Weimer wrote:
>> * Xi Ruoyao:
>> 
>>> The _nocancel functions returns errors as negative values, i. e. -EINTR
>>> for interrupted system call.  But TEMP_FAILURE_RETRY expects the errors
>>> as errno, thus using TEMP_FAILURE_RETRY is incorrect here.
>>>
>>> Add a customized TEMP_FAILURE_RETRY_NEG macro and use it instead of
>>> TEMP_FAILURE_RETRY to fix the issue.
>> 
>> Or we could change __getrandom_nocancel to use INLINE_SYSCALL_CALL
>> instead of INTERNAL_SYCALL_CALL.
>
> It follows what other _nocancel functions that return a value do.  The
> __getrandom_nocancel is also used on malloc initialization, but I do
> not think touching errno should be an issue.

It's inconsistent.  Most of the implementations in
sysdeps/unix/sysv/linux/*_nocancel*.c use INLINE_SYSCALL_CALL, and
therefore set errno.

Thanks,
Florian
Adhemerval Zanella Netto Jan. 4, 2024, 12:31 p.m. UTC | #4 
On 04/01/24 09:16, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
> 
>> On 04/01/24 06:51, Florian Weimer wrote:
>>> * Xi Ruoyao:
>>>
>>>> The _nocancel functions returns errors as negative values, i. e. -EINTR
>>>> for interrupted system call.  But TEMP_FAILURE_RETRY expects the errors
>>>> as errno, thus using TEMP_FAILURE_RETRY is incorrect here.
>>>>
>>>> Add a customized TEMP_FAILURE_RETRY_NEG macro and use it instead of
>>>> TEMP_FAILURE_RETRY to fix the issue.
>>>
>>> Or we could change __getrandom_nocancel to use INLINE_SYSCALL_CALL
>>> instead of INTERNAL_SYCALL_CALL.
>>
>> It follows what other _nocancel functions that return a value do.  The
>> __getrandom_nocancel is also used on malloc initialization, but I do
>> not think touching errno should be an issue.
> 
> It's inconsistent.  Most of the implementations in
> sysdeps/unix/sysv/linux/*_nocancel*.c use INLINE_SYSCALL_CALL, and
> therefore set errno.

I meant that *your suggestion* follow what other _nocancel functions that 
return a value do (sorry for the miscommunication).
Xi Ruoyao Jan. 4, 2024, 12:45 p.m. UTC | #5 
On Thu, 2024-01-04 at 09:31 -0300, Adhemerval Zanella Netto wrote:
> 
> 
> On 04/01/24 09:16, Florian Weimer wrote:
> > * Adhemerval Zanella Netto:
> > 
> > > On 04/01/24 06:51, Florian Weimer wrote:
> > > > * Xi Ruoyao:
> > > > 
> > > > > The _nocancel functions returns errors as negative values, i. e. -EINTR
> > > > > for interrupted system call.  But TEMP_FAILURE_RETRY expects the errors
> > > > > as errno, thus using TEMP_FAILURE_RETRY is incorrect here.
> > > > > 
> > > > > Add a customized TEMP_FAILURE_RETRY_NEG macro and use it instead of
> > > > > TEMP_FAILURE_RETRY to fix the issue.
> > > > 
> > > > Or we could change __getrandom_nocancel to use INLINE_SYSCALL_CALL
> > > > instead of INTERNAL_SYCALL_CALL.
> > > 
> > > It follows what other _nocancel functions that return a value do.  The
> > > __getrandom_nocancel is also used on malloc initialization, but I do
> > > not think touching errno should be an issue.
> > 
> > It's inconsistent.  Most of the implementations in
> > sysdeps/unix/sysv/linux/*_nocancel*.c use INLINE_SYSCALL_CALL, and
> > therefore set errno.
> 
> I meant that *your suggestion* follow what other _nocancel functions that 
> return a value do (sorry for the miscommunication). 

Phew, I mistakenly assumed all _nocancel functions did not use errno. 
So this patch is wrong and I'll make a V2.
diff mbox series

Patch

diff --git a/stdlib/arc4random.c b/stdlib/arc4random.c
index 3ae8fc1302..41f24c124c 100644
--- a/stdlib/arc4random.c
+++ b/stdlib/arc4random.c
@@ -30,6 +30,15 @@  arc4random_getrandom_failure (void)
   __libc_fatal ("Fatal glibc error: cannot get entropy for arc4random\n");
 }
 
+/* Special version of TEMP_FAILURE_RETRY for error values as negative
+   values.  */
+#define TEMP_FAILURE_RETRY_NEG(expression)	\
+  (__extension__				\
+    ({ long int __result;			\
+       do __result = (long int) (expression);	\
+       while (__result == -EINTR);		\
+       __result; }))
+
 void
 __arc4random_buf (void *p, size_t n)
 {
@@ -42,7 +51,7 @@  __arc4random_buf (void *p, size_t n)
 
   for (;;)
     {
-      l = TEMP_FAILURE_RETRY (__getrandom_nocancel (p, n, 0));
+      l = TEMP_FAILURE_RETRY_NEG (__getrandom_nocancel (p, n, 0));
       if (l > 0)
 	{
 	  if ((size_t) l == n)
@@ -60,24 +69,24 @@  __arc4random_buf (void *p, size_t n)
     {
       /* Poll /dev/random as an approximation of RNG initialization.  */
       struct pollfd pfd = { .events = POLLIN };
-      pfd.fd = TEMP_FAILURE_RETRY (
+      pfd.fd = TEMP_FAILURE_RETRY_NEG (
 	  __open64_nocancel ("/dev/random", O_RDONLY | O_CLOEXEC | O_NOCTTY));
       if (pfd.fd < 0)
 	arc4random_getrandom_failure ();
-      if (TEMP_FAILURE_RETRY (__poll_infinity_nocancel (&pfd, 1)) < 0)
+      if (TEMP_FAILURE_RETRY_NEG (__poll_infinity_nocancel (&pfd, 1)) < 0)
 	arc4random_getrandom_failure ();
       if (__close_nocancel (pfd.fd) < 0)
 	arc4random_getrandom_failure ();
       atomic_store_relaxed (&seen_initialized, 1);
     }
 
-  fd = TEMP_FAILURE_RETRY (
+  fd = TEMP_FAILURE_RETRY_NEG (
       __open64_nocancel ("/dev/urandom", O_RDONLY | O_CLOEXEC | O_NOCTTY));
   if (fd < 0)
     arc4random_getrandom_failure ();
   for (;;)
     {
-      l = TEMP_FAILURE_RETRY (__read_nocancel (fd, p, n));
+      l = TEMP_FAILURE_RETRY_NEG (__read_nocancel (fd, p, n));
       if (l <= 0)
 	arc4random_getrandom_failure ();
       if ((size_t) l == n)