* [PATCH] posix: Fix -Warray-bounds instances building timer_create [BZ #26687]
@ 2020-10-05 21:12 Adhemerval Zanella
2020-10-06 8:13 ` Florian Weimer
2020-10-06 11:27 ` Florian Weimer
0 siblings, 2 replies; 9+ messages in thread
From: Adhemerval Zanella @ 2020-10-05 21:12 UTC (permalink / raw)
To: libc-alpha
GCC 11 -Warray-bounds triggers invalid warnings when building
Linux timer_create.c:
../sysdeps/unix/sysv/linux/timer_create.c: In function ‘__timer_create_new’:
../sysdeps/unix/sysv/linux/timer_create.c:83:17: warning: array subscript ‘struct timer[0]’ is partly outside array bounds of ‘unsigned char[8]’ [-Warray-bounds]
83 | newp->sigev_notify = (evp != NULL
| ^~
../sysdeps/unix/sysv/linux/timer_create.c:59:47: note: referencing an object of size 8 allocated by ‘malloc’
59 | struct timer *newp = (struct timer *) malloc (offsetof (struct timer,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
60 | thrfunc));
| ~~~~~~~~~
The struct allocated for !SIGEV_THREAD timers only requires two 'int'
fields (sigev_notify and ktimerid) and the offsetof trick tries minimize
the memory usage by only allocation the required size. However,
although the resulting size is suffice for !SIGEV_THREAD time, accesing
the partially allocated object is error-prone and UB.
This patch fixes both issues by embedding the information whether
the timer if a SIGEV_THREAD in the returned 'timer_t'. For
!SIGEV_THREAD, the resulting 'timer_t' is the returned kernel timer
identifer (kernel_timer_t), while for SIGEV_THREAD it uses the fact
malloc returns at least _Alignof (max_align_t) pointers plus that
valid kernel_timer_t are always positive to set MSB bit of the returned
'timer_t' to indicate the timer handles a SIGEV_THREAD.
It allows to remove the memory allocation for !SIGEV_THREAD and also
remove the 'sigev_notify' field from 'struct timer'.
Checked on x86_64-linux-gnu and i686-linux-gnu.
---
sysdeps/unix/sysv/linux/kernel-posix-timers.h | 46 +++++++++---
sysdeps/unix/sysv/linux/timer_create.c | 74 ++++++-------------
sysdeps/unix/sysv/linux/timer_delete.c | 15 ++--
sysdeps/unix/sysv/linux/timer_getoverr.c | 8 +-
sysdeps/unix/sysv/linux/timer_gettime.c | 6 +-
sysdeps/unix/sysv/linux/timer_settime.c | 8 +-
.../unix/sysv/linux/x86_64/timer_gettime.c | 4 +-
.../unix/sysv/linux/x86_64/timer_settime.c | 5 +-
8 files changed, 77 insertions(+), 89 deletions(-)
diff --git a/sysdeps/unix/sysv/linux/kernel-posix-timers.h b/sysdeps/unix/sysv/linux/kernel-posix-timers.h
index 4f8d97d3de..2a5ee3d0fb 100644
--- a/sysdeps/unix/sysv/linux/kernel-posix-timers.h
+++ b/sysdeps/unix/sysv/linux/kernel-posix-timers.h
@@ -43,21 +43,11 @@ extern pthread_mutex_t __active_timer_sigev_thread_lock attribute_hidden;
/* Type of timers in the kernel. */
typedef int kernel_timer_t;
-
-/* Internal representation of timer. */
+/* Internal representation of SIGEV_THREAD timer. */
struct timer
{
- /* Notification mechanism. */
- int sigev_notify;
-
- /* Timer ID returned by the kernel. */
kernel_timer_t ktimerid;
- /* All new elements must be added after ktimerid. And if the thrfunc
- element is not the third element anymore the memory allocation in
- timer_create needs to be changed. */
-
- /* Parameters for the thread to be started for SIGEV_THREAD. */
void (*thrfunc) (sigval_t);
sigval_t sival;
pthread_attr_t attr;
@@ -65,3 +55,37 @@ struct timer
/* Next element in list of active SIGEV_THREAD timers. */
struct timer *next;
};
+
+
+/* For !SIGEV_THREAD, the resulting 'timer_t' is the returned kernel timer
+ identifer (kernel_timer_t), while for SIGEV_THREAD it uses the fact malloc
+ returns at least _Alignof (max_align_t) pointers plus that valid
+ kernel_timer_t are always positive to set MSB bit of the returned 'timer_t'
+ to indicate the timer handles a SIGEV_THREAD. */
+
+static inline timer_t
+timer_to_timerid (struct timer *ptr)
+{
+ return (timer_t) (INTPTR_MIN | (uintptr_t) ptr >> 1);
+}
+
+static inline bool
+timer_is_sigev_thread (timer_t timerid)
+{
+ return (intptr_t) timerid < 0;
+}
+
+static inline struct timer *
+timerid_to_timer (timer_t timerid)
+{
+ return (struct timer *)((uintptr_t) timerid << 1);
+}
+
+static inline kernel_timer_t
+timerid_to_kernel_timer (timer_t timerid)
+{
+ if (timer_is_sigev_thread (timerid))
+ return timerid_to_timer (timerid)->ktimerid;
+ else
+ return (kernel_timer_t) ((uintptr_t) timerid);
+}
diff --git a/sysdeps/unix/sysv/linux/timer_create.c b/sysdeps/unix/sysv/linux/timer_create.c
index 370c99a517..69b4e667fa 100644
--- a/sysdeps/unix/sysv/linux/timer_create.c
+++ b/sysdeps/unix/sysv/linux/timer_create.c
@@ -52,16 +52,6 @@ timer_create (clockid_t clock_id, struct sigevent *evp, timer_t *timerid)
{
struct sigevent local_evp;
- /* We avoid allocating too much memory by basically
- using struct timer as a derived class with the
- first two elements being in the superclass. We only
- need these two elements here. */
- struct timer *newp = (struct timer *) malloc (offsetof (struct timer,
- thrfunc));
- if (newp == NULL)
- /* No more memory. */
- return -1;
-
if (evp == NULL)
{
/* The kernel has to pass up the timer ID which is a
@@ -69,31 +59,17 @@ timer_create (clockid_t clock_id, struct sigevent *evp, timer_t *timerid)
the kernel to determine it. */
local_evp.sigev_notify = SIGEV_SIGNAL;
local_evp.sigev_signo = SIGALRM;
- local_evp.sigev_value.sival_ptr = newp;
+ local_evp.sigev_value.sival_ptr = NULL;
evp = &local_evp;
}
kernel_timer_t ktimerid;
- int retval = INLINE_SYSCALL (timer_create, 3, syscall_clockid, evp,
- &ktimerid);
-
- if (retval != -1)
- {
- newp->sigev_notify = (evp != NULL
- ? evp->sigev_notify : SIGEV_SIGNAL);
- newp->ktimerid = ktimerid;
-
- *timerid = (timer_t) newp;
- }
- else
- {
- /* Cannot allocate the timer, fail. */
- free (newp);
- retval = -1;
- }
+ if (INLINE_SYSCALL_CALL (timer_create, syscall_clockid, evp,
+ &ktimerid) == -1)
+ return -1;
- return retval;
+ *timerid = (timer_t) ((intptr_t) ktimerid);
}
else
{
@@ -106,20 +82,18 @@ timer_create (clockid_t clock_id, struct sigevent *evp, timer_t *timerid)
return -1;
}
- struct timer *newp;
- newp = (struct timer *) malloc (sizeof (struct timer));
+ struct timer *newp = malloc (sizeof (struct timer));
if (newp == NULL)
return -1;
/* Copy the thread parameters the user provided. */
newp->sival = evp->sigev_value;
newp->thrfunc = evp->sigev_notify_function;
- newp->sigev_notify = SIGEV_THREAD;
/* We cannot simply copy the thread attributes since the
implementation might keep internal information for
each instance. */
- (void) pthread_attr_init (&newp->attr);
+ pthread_attr_init (&newp->attr);
if (evp->sigev_notify_attributes != NULL)
{
struct pthread_attr *nattr;
@@ -137,8 +111,7 @@ timer_create (clockid_t clock_id, struct sigevent *evp, timer_t *timerid)
}
/* In any case set the detach flag. */
- (void) pthread_attr_setdetachstate (&newp->attr,
- PTHREAD_CREATE_DETACHED);
+ pthread_attr_setdetachstate (&newp->attr, PTHREAD_CREATE_DETACHED);
/* Create the event structure for the kernel timer. */
struct sigevent sev =
@@ -149,27 +122,24 @@ timer_create (clockid_t clock_id, struct sigevent *evp, timer_t *timerid)
/* Create the timer. */
int res;
- res = INTERNAL_SYSCALL_CALL (timer_create,
- syscall_clockid, &sev, &newp->ktimerid);
- if (! INTERNAL_SYSCALL_ERROR_P (res))
+ res = INTERNAL_SYSCALL_CALL (timer_create, syscall_clockid, &sev,
+ &newp->ktimerid);
+ if (res == -1)
{
- /* Add to the queue of active timers with thread
- delivery. */
- pthread_mutex_lock (&__active_timer_sigev_thread_lock);
- newp->next = __active_timer_sigev_thread;
- __active_timer_sigev_thread = newp;
- pthread_mutex_unlock (&__active_timer_sigev_thread_lock);
-
- *timerid = (timer_t) newp;
- return 0;
+ free (newp);
+ __set_errno (INTERNAL_SYSCALL_ERRNO (res));
+ return -1;
}
- /* Free the resources. */
- free (newp);
-
- __set_errno (INTERNAL_SYSCALL_ERRNO (res));
+ /* Add to the queue of active timers with thread delivery. */
+ pthread_mutex_lock (&__active_timer_sigev_thread_lock);
+ newp->next = __active_timer_sigev_thread;
+ __active_timer_sigev_thread = newp;
+ pthread_mutex_unlock (&__active_timer_sigev_thread_lock);
- return -1;
+ *timerid = timer_to_timerid (newp);
}
}
+
+ return 0;
}
diff --git a/sysdeps/unix/sysv/linux/timer_delete.c b/sysdeps/unix/sysv/linux/timer_delete.c
index c240c06139..c8a5f101b4 100644
--- a/sysdeps/unix/sysv/linux/timer_delete.c
+++ b/sysdeps/unix/sysv/linux/timer_delete.c
@@ -32,15 +32,15 @@ int
timer_delete (timer_t timerid)
{
#undef timer_delete
- struct timer *kt = (struct timer *) timerid;
-
- /* Delete the kernel timer object. */
- int res = INLINE_SYSCALL (timer_delete, 1, kt->ktimerid);
+ kernel_timer_t ktimerid = timerid_to_kernel_timer (timerid);
+ int res = INLINE_SYSCALL_CALL (timer_delete, ktimerid);
if (res == 0)
{
- if (kt->sigev_notify == SIGEV_THREAD)
+ if (timer_is_sigev_thread (timerid))
{
+ struct timer *kt = timerid_to_timer (timerid);
+
/* Remove the timer from the list. */
pthread_mutex_lock (&__active_timer_sigev_thread_lock);
if (__active_timer_sigev_thread == kt)
@@ -58,10 +58,9 @@ timer_delete (timer_t timerid)
prevp = prevp->next;
}
pthread_mutex_unlock (&__active_timer_sigev_thread_lock);
- }
- /* Free the memory. */
- (void) free (kt);
+ free (kt);
+ }
return 0;
}
diff --git a/sysdeps/unix/sysv/linux/timer_getoverr.c b/sysdeps/unix/sysv/linux/timer_getoverr.c
index 81b9723f01..7862d162b9 100644
--- a/sysdeps/unix/sysv/linux/timer_getoverr.c
+++ b/sysdeps/unix/sysv/linux/timer_getoverr.c
@@ -31,10 +31,6 @@ int
timer_getoverrun (timer_t timerid)
{
#undef timer_getoverrun
- struct timer *kt = (struct timer *) timerid;
-
- /* Get the information from the kernel. */
- int res = INLINE_SYSCALL (timer_getoverrun, 1, kt->ktimerid);
-
- return res;
+ kernel_timer_t ktimerid = timerid_to_kernel_timer (timerid);
+ return INLINE_SYSCALL_CALL (timer_getoverrun, ktimerid);
}
diff --git a/sysdeps/unix/sysv/linux/timer_gettime.c b/sysdeps/unix/sysv/linux/timer_gettime.c
index 5d31c7c864..07b6309ba9 100644
--- a/sysdeps/unix/sysv/linux/timer_gettime.c
+++ b/sysdeps/unix/sysv/linux/timer_gettime.c
@@ -26,18 +26,18 @@
int
__timer_gettime64 (timer_t timerid, struct __itimerspec64 *value)
{
- struct timer *kt = (struct timer *) timerid;
+ kernel_timer_t ktimerid = timerid_to_kernel_timer (timerid);
#ifndef __NR_timer_gettime64
# define __NR_timer_gettime64 __NR_timer_gettime
#endif
- int ret = INLINE_SYSCALL_CALL (timer_gettime64, kt->ktimerid, value);
+ int ret = INLINE_SYSCALL_CALL (timer_gettime64, ktimerid, value);
#ifndef __ASSUME_TIME64_SYSCALLS
if (ret == 0 || errno != ENOSYS)
return ret;
struct itimerspec its32;
- ret = INLINE_SYSCALL_CALL (timer_gettime, kt->ktimerid, &its32);
+ ret = INLINE_SYSCALL_CALL (timer_gettime, ktimerid, &its32);
if (ret == 0)
{
value->it_interval = valid_timespec_to_timespec64 (its32.it_interval);
diff --git a/sysdeps/unix/sysv/linux/timer_settime.c b/sysdeps/unix/sysv/linux/timer_settime.c
index c97a9e92ef..01c6257938 100644
--- a/sysdeps/unix/sysv/linux/timer_settime.c
+++ b/sysdeps/unix/sysv/linux/timer_settime.c
@@ -28,17 +28,17 @@ __timer_settime64 (timer_t timerid, int flags,
const struct __itimerspec64 *value,
struct __itimerspec64 *ovalue)
{
- struct timer *kt = (struct timer *) timerid;
+ kernel_timer_t ktimerid = timerid_to_kernel_timer (timerid);
#ifdef __ASSUME_TIME64_SYSCALLS
# ifndef __NR_timer_settime64
# define __NR_timer_settime64 __NR_timer_settime
# endif
- return INLINE_SYSCALL_CALL (timer_settime64, kt->ktimerid, flags, value,
+ return INLINE_SYSCALL_CALL (timer_settime64, ktimerid, flags, value,
ovalue);
#else
# ifdef __NR_timer_settime64
- int ret = INLINE_SYSCALL_CALL (timer_settime64, kt->ktimerid, flags, value,
+ int ret = INLINE_SYSCALL_CALL (timer_settime64, ktimerid, flags, value,
ovalue);
if (ret == 0 || errno != ENOSYS)
return ret;
@@ -55,7 +55,7 @@ __timer_settime64 (timer_t timerid, int flags,
its32.it_interval = valid_timespec64_to_timespec (value->it_interval);
its32.it_value = valid_timespec64_to_timespec (value->it_value);
- int retval = INLINE_SYSCALL_CALL (timer_settime, kt->ktimerid, flags,
+ int retval = INLINE_SYSCALL_CALL (timer_settime, ktimerid, flags,
&its32, ovalue ? &oits32 : NULL);
if (retval == 0 && ovalue)
{
diff --git a/sysdeps/unix/sysv/linux/x86_64/timer_gettime.c b/sysdeps/unix/sysv/linux/x86_64/timer_gettime.c
index 89002c7243..0e4213d7aa 100644
--- a/sysdeps/unix/sysv/linux/x86_64/timer_gettime.c
+++ b/sysdeps/unix/sysv/linux/x86_64/timer_gettime.c
@@ -24,9 +24,9 @@
int
__timer_gettime_new (timer_t timerid, struct itimerspec *value)
{
- struct timer *kt = (struct timer *) timerid;
+ kernel_timer_t ktimerid = timerid_to_kernel_timer (timerid);
- return INLINE_SYSCALL_CALL (timer_gettime, kt->ktimerid, value);
+ return INLINE_SYSCALL_CALL (timer_gettime, ktimerid, value);
}
versioned_symbol (librt, __timer_gettime_new, timer_gettime, GLIBC_2_3_3);
diff --git a/sysdeps/unix/sysv/linux/x86_64/timer_settime.c b/sysdeps/unix/sysv/linux/x86_64/timer_settime.c
index 7af417459e..0636f7316d 100644
--- a/sysdeps/unix/sysv/linux/x86_64/timer_settime.c
+++ b/sysdeps/unix/sysv/linux/x86_64/timer_settime.c
@@ -25,10 +25,9 @@ int
__timer_settime_new (timer_t timerid, int flags, const struct itimerspec *value,
struct itimerspec *ovalue)
{
- struct timer *kt = (struct timer *) timerid;
+ kernel_timer_t ktimerid = timerid_to_kernel_timer (timerid);
- return INLINE_SYSCALL_CALL (timer_settime, kt->ktimerid, flags, value,
- ovalue);
+ return INLINE_SYSCALL_CALL (timer_settime, ktimerid, flags, value, ovalue);
}
versioned_symbol (librt, __timer_settime_new, timer_settime, GLIBC_2_3_3);
--
2.25.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] posix: Fix -Warray-bounds instances building timer_create [BZ #26687]
2020-10-05 21:12 [PATCH] posix: Fix -Warray-bounds instances building timer_create [BZ #26687] Adhemerval Zanella
@ 2020-10-06 8:13 ` Florian Weimer
2020-10-06 8:21 ` Florian Weimer
2020-10-06 11:27 ` Florian Weimer
1 sibling, 1 reply; 9+ messages in thread
From: Florian Weimer @ 2020-10-06 8:13 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha
* Adhemerval Zanella via Libc-alpha:
> This patch fixes both issues by embedding the information whether
> the timer if a SIGEV_THREAD in the returned 'timer_t'. For
> !SIGEV_THREAD, the resulting 'timer_t' is the returned kernel timer
> identifer (kernel_timer_t), while for SIGEV_THREAD it uses the fact
> malloc returns at least _Alignof (max_align_t) pointers plus that
> valid kernel_timer_t are always positive to set MSB bit of the returned
> 'timer_t' to indicate the timer handles a SIGEV_THREAD.
LSB, not MSB, right?
Thanks,
Florian
--
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] posix: Fix -Warray-bounds instances building timer_create [BZ #26687]
2020-10-06 8:13 ` Florian Weimer
@ 2020-10-06 8:21 ` Florian Weimer
2020-10-06 14:00 ` Adhemerval Zanella
0 siblings, 1 reply; 9+ messages in thread
From: Florian Weimer @ 2020-10-06 8:21 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha
* Florian Weimer:
> * Adhemerval Zanella via Libc-alpha:
>
>> This patch fixes both issues by embedding the information whether
>> the timer if a SIGEV_THREAD in the returned 'timer_t'. For
>> !SIGEV_THREAD, the resulting 'timer_t' is the returned kernel timer
>> identifer (kernel_timer_t), while for SIGEV_THREAD it uses the fact
>> malloc returns at least _Alignof (max_align_t) pointers plus that
>> valid kernel_timer_t are always positive to set MSB bit of the returned
>> 'timer_t' to indicate the timer handles a SIGEV_THREAD.
>
> LSB, not MSB, right?
Ah, no, it is the MSB.
I think using the LSB is generally more efficient for this because it is
easier to manipulate. In this scenario, pointers would be unencoded,
and a kernel_timer_t value ktid would be encoded as 2 * kdid + 1. But I
guess the difference does not matter much.
Thanks,
Florian
--
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] posix: Fix -Warray-bounds instances building timer_create [BZ #26687]
2020-10-06 8:21 ` Florian Weimer
@ 2020-10-06 14:00 ` Adhemerval Zanella
2020-10-06 14:13 ` Florian Weimer
0 siblings, 1 reply; 9+ messages in thread
From: Adhemerval Zanella @ 2020-10-06 14:00 UTC (permalink / raw)
To: Florian Weimer, Adhemerval Zanella via Libc-alpha
On 06/10/2020 05:21, Florian Weimer wrote:
> * Florian Weimer:
>
>> * Adhemerval Zanella via Libc-alpha:
>>
>>> This patch fixes both issues by embedding the information whether
>>> the timer if a SIGEV_THREAD in the returned 'timer_t'. For
>>> !SIGEV_THREAD, the resulting 'timer_t' is the returned kernel timer
>>> identifer (kernel_timer_t), while for SIGEV_THREAD it uses the fact
>>> malloc returns at least _Alignof (max_align_t) pointers plus that
>>> valid kernel_timer_t are always positive to set MSB bit of the returned
>>> 'timer_t' to indicate the timer handles a SIGEV_THREAD.
>>
>> LSB, not MSB, right?
>
> Ah, no, it is the MSB.
>
> I think using the LSB is generally more efficient for this because it is
> easier to manipulate. In this scenario, pointers would be unencoded,
> and a kernel_timer_t value ktid would be encoded as 2 * kdid + 1. But I
> guess the difference does not matter much.
I think either way works and it should be easy to change since all the
logic to convert from 'timer_t' to 'kernel_timer_t' and check if the
timer is a SIGEV_THREAD is at kernel-posix-timers.h.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] posix: Fix -Warray-bounds instances building timer_create [BZ #26687]
2020-10-06 14:00 ` Adhemerval Zanella
@ 2020-10-06 14:13 ` Florian Weimer
2020-10-06 14:24 ` Adhemerval Zanella
0 siblings, 1 reply; 9+ messages in thread
From: Florian Weimer @ 2020-10-06 14:13 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: Adhemerval Zanella via Libc-alpha
* Adhemerval Zanella:
> On 06/10/2020 05:21, Florian Weimer wrote:
>> * Florian Weimer:
>>
>>> * Adhemerval Zanella via Libc-alpha:
>>>
>>>> This patch fixes both issues by embedding the information whether
>>>> the timer if a SIGEV_THREAD in the returned 'timer_t'. For
>>>> !SIGEV_THREAD, the resulting 'timer_t' is the returned kernel timer
>>>> identifer (kernel_timer_t), while for SIGEV_THREAD it uses the fact
>>>> malloc returns at least _Alignof (max_align_t) pointers plus that
>>>> valid kernel_timer_t are always positive to set MSB bit of the returned
>>>> 'timer_t' to indicate the timer handles a SIGEV_THREAD.
>>>
>>> LSB, not MSB, right?
>>
>> Ah, no, it is the MSB.
>>
>> I think using the LSB is generally more efficient for this because it is
>> easier to manipulate. In this scenario, pointers would be unencoded,
>> and a kernel_timer_t value ktid would be encoded as 2 * kdid + 1. But I
>> guess the difference does not matter much.
>
> I think either way works and it should be easy to change since all the
> logic to convert from 'timer_t' to 'kernel_timer_t' and check if the
> timer is a SIGEV_THREAD is at kernel-posix-timers.h.
Except the kernel-ID-to-timer_t translation. The series of casts is
currently open-coded. Maybe you could add an inline helper function for
that, too?
Thanks,
Florian
--
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] posix: Fix -Warray-bounds instances building timer_create [BZ #26687]
2020-10-06 14:13 ` Florian Weimer
@ 2020-10-06 14:24 ` Adhemerval Zanella
0 siblings, 0 replies; 9+ messages in thread
From: Adhemerval Zanella @ 2020-10-06 14:24 UTC (permalink / raw)
To: Florian Weimer; +Cc: Adhemerval Zanella via Libc-alpha
On 06/10/2020 11:13, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> On 06/10/2020 05:21, Florian Weimer wrote:
>>> * Florian Weimer:
>>>
>>>> * Adhemerval Zanella via Libc-alpha:
>>>>
>>>>> This patch fixes both issues by embedding the information whether
>>>>> the timer if a SIGEV_THREAD in the returned 'timer_t'. For
>>>>> !SIGEV_THREAD, the resulting 'timer_t' is the returned kernel timer
>>>>> identifer (kernel_timer_t), while for SIGEV_THREAD it uses the fact
>>>>> malloc returns at least _Alignof (max_align_t) pointers plus that
>>>>> valid kernel_timer_t are always positive to set MSB bit of the returned
>>>>> 'timer_t' to indicate the timer handles a SIGEV_THREAD.
>>>>
>>>> LSB, not MSB, right?
>>>
>>> Ah, no, it is the MSB.
>>>
>>> I think using the LSB is generally more efficient for this because it is
>>> easier to manipulate. In this scenario, pointers would be unencoded,
>>> and a kernel_timer_t value ktid would be encoded as 2 * kdid + 1. But I
>>> guess the difference does not matter much.
>>
>> I think either way works and it should be easy to change since all the
>> logic to convert from 'timer_t' to 'kernel_timer_t' and check if the
>> timer is a SIGEV_THREAD is at kernel-posix-timers.h.
>
> Except the kernel-ID-to-timer_t translation. The series of casts is
> currently open-coded. Maybe you could add an inline helper function for
> that, too?
Ok, it sounds reasonable. I will add them and push upstream.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] posix: Fix -Warray-bounds instances building timer_create [BZ #26687]
2020-10-05 21:12 [PATCH] posix: Fix -Warray-bounds instances building timer_create [BZ #26687] Adhemerval Zanella
2020-10-06 8:13 ` Florian Weimer
@ 2020-10-06 11:27 ` Florian Weimer
2020-10-06 13:57 ` Adhemerval Zanella
1 sibling, 1 reply; 9+ messages in thread
From: Florian Weimer @ 2020-10-06 11:27 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha
* Adhemerval Zanella via Libc-alpha:
> diff --git a/sysdeps/unix/sysv/linux/timer_create.c b/sysdeps/unix/sysv/linux/timer_create.c
> index 370c99a517..69b4e667fa 100644
> --- a/sysdeps/unix/sysv/linux/timer_create.c
> +++ b/sysdeps/unix/sysv/linux/timer_create.c
> @@ -149,27 +122,24 @@ timer_create (clockid_t clock_id, struct sigevent *evp, timer_t *timerid)
>
> /* Create the timer. */
> int res;
> + res = INTERNAL_SYSCALL_CALL (timer_create, syscall_clockid, &sev,
> + &newp->ktimerid);
> + if (res == -1)
> {
> + free (newp);
> + __set_errno (INTERNAL_SYSCALL_ERRNO (res));
> + return -1;
> }
This does not look like a correct use of INTERNAL_SYSCALL_CALL to me.
Don't you have to use INTERNAL_SYSCALL_ERROR_P here?
Thanks,
Florian
--
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] posix: Fix -Warray-bounds instances building timer_create [BZ #26687]
2020-10-06 11:27 ` Florian Weimer
@ 2020-10-06 13:57 ` Adhemerval Zanella
2020-10-06 14:14 ` Florian Weimer
0 siblings, 1 reply; 9+ messages in thread
From: Adhemerval Zanella @ 2020-10-06 13:57 UTC (permalink / raw)
To: Florian Weimer, Adhemerval Zanella via Libc-alpha
On 06/10/2020 08:27, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
>
>> diff --git a/sysdeps/unix/sysv/linux/timer_create.c b/sysdeps/unix/sysv/linux/timer_create.c
>> index 370c99a517..69b4e667fa 100644
>> --- a/sysdeps/unix/sysv/linux/timer_create.c
>> +++ b/sysdeps/unix/sysv/linux/timer_create.c
>> @@ -149,27 +122,24 @@ timer_create (clockid_t clock_id, struct sigevent *evp, timer_t *timerid)
>>
>> /* Create the timer. */
>> int res;
>> + res = INTERNAL_SYSCALL_CALL (timer_create, syscall_clockid, &sev,
>> + &newp->ktimerid);
>> + if (res == -1)
>> {
>> + free (newp);
>> + __set_errno (INTERNAL_SYSCALL_ERRNO (res));
>> + return -1;
>> }
>
> This does not look like a correct use of INTERNAL_SYSCALL_CALL to me.
> Don't you have to use INTERNAL_SYSCALL_ERROR_P here?
It should indeed, I have changed to use INTERNAL_SYSCALL_ERROR_P instead
of using comparing to '-1'. Are you ok with this patch with this change?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] posix: Fix -Warray-bounds instances building timer_create [BZ #26687]
2020-10-06 13:57 ` Adhemerval Zanella
@ 2020-10-06 14:14 ` Florian Weimer
0 siblings, 0 replies; 9+ messages in thread
From: Florian Weimer @ 2020-10-06 14:14 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: Adhemerval Zanella via Libc-alpha
* Adhemerval Zanella:
> On 06/10/2020 08:27, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>>
>>> diff --git a/sysdeps/unix/sysv/linux/timer_create.c b/sysdeps/unix/sysv/linux/timer_create.c
>>> index 370c99a517..69b4e667fa 100644
>>> --- a/sysdeps/unix/sysv/linux/timer_create.c
>>> +++ b/sysdeps/unix/sysv/linux/timer_create.c
>>> @@ -149,27 +122,24 @@ timer_create (clockid_t clock_id, struct sigevent *evp, timer_t *timerid)
>>>
>>> /* Create the timer. */
>>> int res;
>>> + res = INTERNAL_SYSCALL_CALL (timer_create, syscall_clockid, &sev,
>>> + &newp->ktimerid);
>>> + if (res == -1)
>>> {
>>> + free (newp);
>>> + __set_errno (INTERNAL_SYSCALL_ERRNO (res));
>>> + return -1;
>>> }
>>
>> This does not look like a correct use of INTERNAL_SYSCALL_CALL to me.
>> Don't you have to use INTERNAL_SYSCALL_ERROR_P here?
>
> It should indeed, I have changed to use INTERNAL_SYSCALL_ERROR_P instead
> of using comparing to '-1'. Are you ok with this patch with this change?
See my other message. Maybe you can add another converison function?
But the rest looks okay to me, thanks.
Florian
--
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-10-06 14:24 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-05 21:12 [PATCH] posix: Fix -Warray-bounds instances building timer_create [BZ #26687] Adhemerval Zanella
2020-10-06 8:13 ` Florian Weimer
2020-10-06 8:21 ` Florian Weimer
2020-10-06 14:00 ` Adhemerval Zanella
2020-10-06 14:13 ` Florian Weimer
2020-10-06 14:24 ` Adhemerval Zanella
2020-10-06 11:27 ` Florian Weimer
2020-10-06 13:57 ` Adhemerval Zanella
2020-10-06 14:14 ` Florian Weimer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).