| Message ID | e2a6e000b123c4e5ddacfce1511a364b509b9ea1.1618301209.git.szabolcs.nagy@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Dynamic TLS related data race fixes | expand |
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.
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])
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.
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).
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 --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;
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(-)