posix: Fix -Warray-bounds instances building timer_create [BZ #26687]
Commit Message
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(-)
Comments
* 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
* 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
* 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
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?
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.
* 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
* 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
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.
@@ -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);
+}
@@ -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;
}
@@ -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;
}
@@ -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);
}
@@ -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);
@@ -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)
{
@@ -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);
@@ -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);