diff mbox series

[v2,01/14] elf: Fix a DTV setup issue [BZ #27136]

Message ID e2a6e000b123c4e5ddacfce1511a364b509b9ea1.1618301209.git.szabolcs.nagy@arm.com
State Committed
Commit d2b997c7172e9a00895a9deb379f8782fbd2e36f
Delegated to: Adhemerval Zanella Netto
Headers
Series Dynamic TLS related data race fixes |

Commit Message

Szabolcs Nagy April 13, 2021, 8:18 a.m. UTC 
  The max modid is a valid index in the dtv, it should not be skipped.

The bug is observable if the last module has modid == 64 and its
generation is same or less than the max generation of the previous
modules.  Then dtv[0].counter implies dtv[64] is initialized but
it isn't. Fixes bug 27136.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
---
 elf/dl-tls.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andreas Schwab April 13, 2021, 8:36 a.m. UTC | #1 
On Apr 13 2021, Szabolcs Nagy via Libc-alpha wrote:

> The max modid is a valid index in the dtv, it should not be skipped.

Does this check in _dl_allocate_tls_init need to be adjusted as well?

  /* Check if the current dtv is big enough.   */
  if (dtv[-1].counter < GL(dl_tls_max_dtv_idx))

Andreas.
Szabolcs Nagy April 13, 2021, 9:35 a.m. UTC | #2 
The 04/13/2021 10:36, Andreas Schwab wrote:
> On Apr 13 2021, Szabolcs Nagy via Libc-alpha wrote:
> 
> > The max modid is a valid index in the dtv, it should not be skipped.
> 
> Does this check in _dl_allocate_tls_init need to be adjusted as well?
> 
>   /* Check if the current dtv is big enough.   */
>   if (dtv[-1].counter < GL(dl_tls_max_dtv_idx))

no, that seems fine because counter is not the dtv length but
the maximum valid index (dtv[dtv[-1].counter] is valid, the
dtv array size is counter+2 to accomodate for dtv[-1] and [0])
Andreas Schwab April 13, 2021, 10:22 a.m. UTC | #3 
On Apr 13 2021, Szabolcs Nagy wrote:

> The 04/13/2021 10:36, Andreas Schwab wrote:
>> On Apr 13 2021, Szabolcs Nagy via Libc-alpha wrote:
>> 
>> > The max modid is a valid index in the dtv, it should not be skipped.
>> 
>> Does this check in _dl_allocate_tls_init need to be adjusted as well?
>> 
>>   /* Check if the current dtv is big enough.   */
>>   if (dtv[-1].counter < GL(dl_tls_max_dtv_idx))
>
> no, that seems fine because counter is not the dtv length but
> the maximum valid index (dtv[dtv[-1].counter] is valid, the
> dtv array size is counter+2 to accomodate for dtv[-1] and [0])

Since both dtv[-1].counter and GL(dl_tls_max_dtv_idx) are indexes, not
lengths, I would expect them to be compared with <=.

Andreas.
Szabolcs Nagy April 13, 2021, 10:34 a.m. UTC | #4 
The 04/13/2021 12:22, Andreas Schwab wrote:
> On Apr 13 2021, Szabolcs Nagy wrote:
> 
> > The 04/13/2021 10:36, Andreas Schwab wrote:
> >> On Apr 13 2021, Szabolcs Nagy via Libc-alpha wrote:
> >> 
> >> > The max modid is a valid index in the dtv, it should not be skipped.
> >> 
> >> Does this check in _dl_allocate_tls_init need to be adjusted as well?
> >> 
> >>   /* Check if the current dtv is big enough.   */
> >>   if (dtv[-1].counter < GL(dl_tls_max_dtv_idx))
> >
> > no, that seems fine because counter is not the dtv length but
> > the maximum valid index (dtv[dtv[-1].counter] is valid, the
> > dtv array size is counter+2 to accomodate for dtv[-1] and [0])
> 
> Since both dtv[-1].counter and GL(dl_tls_max_dtv_idx) are indexes, not
> lengths, I would expect them to be compared with <=.

but the code is

  /* Check if the current dtv is big enough.   */
  if (dtv[-1].counter < GL(dl_tls_max_dtv_idx))
    {
      /* Resize the dtv.  */
      dtv = _dl_resize_dtv (dtv, GL(dl_tls_max_dtv_idx));

in case of == there is no need to resize: the dtv array is
large enough for the observed GL(dl_tls_max_dtv_idx).
Andreas Schwab April 13, 2021, 10:51 a.m. UTC | #5 
On Apr 13 2021, Szabolcs Nagy wrote:

> The 04/13/2021 12:22, Andreas Schwab wrote:
>> On Apr 13 2021, Szabolcs Nagy wrote:
>> 
>> > The 04/13/2021 10:36, Andreas Schwab wrote:
>> >> On Apr 13 2021, Szabolcs Nagy via Libc-alpha wrote:
>> >> 
>> >> > The max modid is a valid index in the dtv, it should not be skipped.
>> >> 
>> >> Does this check in _dl_allocate_tls_init need to be adjusted as well?
>> >> 
>> >>   /* Check if the current dtv is big enough.   */
>> >>   if (dtv[-1].counter < GL(dl_tls_max_dtv_idx))
>> >
>> > no, that seems fine because counter is not the dtv length but
>> > the maximum valid index (dtv[dtv[-1].counter] is valid, the
>> > dtv array size is counter+2 to accomodate for dtv[-1] and [0])
>> 
>> Since both dtv[-1].counter and GL(dl_tls_max_dtv_idx) are indexes, not
>> lengths, I would expect them to be compared with <=.
>
> but the code is
>
>   /* Check if the current dtv is big enough.   */
>   if (dtv[-1].counter < GL(dl_tls_max_dtv_idx))
>     {
>       /* Resize the dtv.  */
>       dtv = _dl_resize_dtv (dtv, GL(dl_tls_max_dtv_idx));
>
> in case of == there is no need to resize: the dtv array is
> large enough for the observed GL(dl_tls_max_dtv_idx).

I think it would be easier to understand if the condition would be
written as (GL(dl_tls_max_dtv_idx) > dtv[-1].counter).

Andreas.
diff mbox series

Patch

diff --git a/elf/dl-tls.c b/elf/dl-tls.c
index dd76829e74..79b93ad91b 100644
--- a/elf/dl-tls.c
+++ b/elf/dl-tls.c
@@ -590,7 +590,7 @@  _dl_allocate_tls_init (void *result)
 	}
 
       total += cnt;
-      if (total >= GL(dl_tls_max_dtv_idx))
+      if (total > GL(dl_tls_max_dtv_idx))
 	break;
 
       listp = listp->next;