malloc: Do not clobber errno on __getrandom_nocancel (BZ#29624)
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
I am not sure if changing thread cache initialization to not use
getrandom syscall will add much, on recent kernel getrandom should
fast (although still a syscall) and I am not sure of the performance
implications.
---
Use INTERNAL_SYSCALL_CALL instead of INLINE_SYSCALL_CALL. This
requires emulate the semantic for hurd call (so __arc4random_buf
uses the fallback).
Checked on x86_64-linux-gnu.
---
stdlib/arc4random.c | 4 ++--
sysdeps/mach/hurd/not-cancel.h | 11 +++++++++--
sysdeps/unix/sysv/linux/not-cancel.h | 2 +-
3 files changed, 12 insertions(+), 5 deletions(-)
Comments
On Sep 29 2022, Adhemerval Zanella via Libc-alpha wrote:
> @@ -51,7 +51,7 @@ __arc4random_buf (void *p, size_t n)
> n -= l;
> continue; /* Interrupted by a signal; keep going. */
> }
> - else if (l < 0 && errno == ENOSYS)
> + else if (l < 0 && l == -ENOSYS)
This is a redundant condition.
Hi,
29 septembre 2022 à 18:45 "Adhemerval Zanella via Libc-alpha" a écrit:
> Use INTERNAL_SYSCALL_CALL instead of INLINE_SYSCALL_CALL. This
> requires emulate the semantic for hurd call (so __arc4random_buf
> uses the fallback).
>
> Checked on x86_64-linux-gnu.
> ---
> stdlib/arc4random.c | 4 ++--
> sysdeps/mach/hurd/not-cancel.h | 11 +++++++++--
> sysdeps/unix/sysv/linux/not-cancel.h | 2 +-
> 3 files changed, 12 insertions(+), 5 deletions(-)
[...]
> diff --git a/sysdeps/mach/hurd/not-cancel.h b/sysdeps/mach/hurd/not-cancel.h
> index ae58b734e3..f2cb9b60ba 100644
> --- a/sysdeps/mach/hurd/not-cancel.h
> +++ b/sysdeps/mach/hurd/not-cancel.h
> @@ -75,8 +76,14 @@ __typeof (__fcntl) __fcntl_nocancel;
> #define __fcntl64_nocancel(...) \
> __fcntl_nocancel (__VA_ARGS__)
>
> -#define __getrandom_nocancel(buf, size, flags) \
> - __getrandom (buf, size, flags)
> +static inline int
> +__getrandom_nocancel (void *buf, size_t buflen, unsigned int flags)
> +{
> + int save_errno = errno;
> + int r = __getrandom (buf, buflen, flags);
> + __set_errno (save_errno);
> + return r == -1 ? -save_errno : r;
> +}
>
I don't get why the saved errno value is returned.
Regards.
On 29/09/22 14:46, Yann Droneaud wrote:
> Hi,
>
> 29 septembre 2022 à 18:45 "Adhemerval Zanella via Libc-alpha" a écrit:
>
>> Use INTERNAL_SYSCALL_CALL instead of INLINE_SYSCALL_CALL. This
>> requires emulate the semantic for hurd call (so __arc4random_buf
>> uses the fallback).
>>
>> Checked on x86_64-linux-gnu.
>> ---
>> stdlib/arc4random.c | 4 ++--
>> sysdeps/mach/hurd/not-cancel.h | 11 +++++++++--
>> sysdeps/unix/sysv/linux/not-cancel.h | 2 +-
>> 3 files changed, 12 insertions(+), 5 deletions(-)
>
> [...]
>
>> diff --git a/sysdeps/mach/hurd/not-cancel.h b/sysdeps/mach/hurd/not-cancel.h
>> index ae58b734e3..f2cb9b60ba 100644
>> --- a/sysdeps/mach/hurd/not-cancel.h
>> +++ b/sysdeps/mach/hurd/not-cancel.h
>> @@ -75,8 +76,14 @@ __typeof (__fcntl) __fcntl_nocancel;
>> #define __fcntl64_nocancel(...) \
>> __fcntl_nocancel (__VA_ARGS__)
>>
>> -#define __getrandom_nocancel(buf, size, flags) \
>> - __getrandom (buf, size, flags)
>> +static inline int
>> +__getrandom_nocancel (void *buf, size_t buflen, unsigned int flags)
>> +{
>> + int save_errno = errno;
>> + int r = __getrandom (buf, buflen, flags);
>> + __set_errno (save_errno);
>> + return r == -1 ? -save_errno : r;
>> +}
>>
>
> I don't get why the saved errno value is returned.
Oops, because it is wrong.
>
>
> Regards.
>
Hi Adhemerval,
+static inline int
+__getrandom_nocancel (void *buf, size_t buflen, unsigned int flags)
+{
+ int save_errno = errno;
+ int r = __getrandom (buf, buflen, flags);
+ __set_errno (save_errno);
+ return r == -1 ? -save_errno : r;
+}
How does this work? If it fails, it returns the negated original errno, which
doesn't make any sense. It can be zero and so it would imply success on
failure... Shouldn't it just return r as it tries to preserve errno or do we need
if (r == -1) r = -errno; before we restore errno?
Cheers,
Wilco
@@ -34,7 +34,7 @@ void
__arc4random_buf (void *p, size_t n)
{
static int seen_initialized;
- size_t l;
+ int l;
int fd;
if (n == 0)
@@ -51,7 +51,7 @@ __arc4random_buf (void *p, size_t n)
n -= l;
continue; /* Interrupted by a signal; keep going. */
}
- else if (l < 0 && errno == ENOSYS)
+ else if (l < 0 && l == -ENOSYS)
break; /* No syscall, so fallback to /dev/urandom. */
arc4random_getrandom_failure ();
}
@@ -27,6 +27,7 @@
#include <sys/uio.h>
#include <hurd.h>
#include <hurd/fd.h>
+#include <sys/random.h>
/* Non cancellable close syscall. */
__typeof (__close) __close_nocancel;
@@ -75,8 +76,14 @@ __typeof (__fcntl) __fcntl_nocancel;
#define __fcntl64_nocancel(...) \
__fcntl_nocancel (__VA_ARGS__)
-#define __getrandom_nocancel(buf, size, flags) \
- __getrandom (buf, size, flags)
+static inline int
+__getrandom_nocancel (void *buf, size_t buflen, unsigned int flags)
+{
+ int save_errno = errno;
+ int r = __getrandom (buf, buflen, flags);
+ __set_errno (save_errno);
+ return r == -1 ? -save_errno : r;
+}
#define __poll_infinity_nocancel(fds, nfds) \
__poll (fds, nfds, -1)
@@ -71,7 +71,7 @@ __writev_nocancel_nostatus (int fd, const struct iovec *iov, int iovcnt)
static inline int
__getrandom_nocancel (void *buf, size_t buflen, unsigned int flags)
{
- return INLINE_SYSCALL_CALL (getrandom, buf, buflen, flags);
+ return INTERNAL_SYSCALL_CALL (getrandom, buf, buflen, flags);
}
static inline int