pkarashchenko commented on a change in pull request #5498: URL: https://github.com/apache/incubator-nuttx/pull/5498#discussion_r810141082
########## File path: sched/task/task_tls_alloc.c ########## @@ -0,0 +1,113 @@ +/**************************************************************************** + * sched/task/task_tls_alloc.c + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. The + * ASF licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the + * License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + ****************************************************************************/ + +/**************************************************************************** + * Included Files + ****************************************************************************/ + +#include <sys/types.h> +#include <unistd.h> +#include <sched.h> +#include <errno.h> + +#include <nuttx/tls.h> +#include <nuttx/semaphore.h> + +/**************************************************************************** + * Private Data + ****************************************************************************/ + +#if CONFIG_TLS_TASK_NELEM > 0 + +static int g_tlsset = 0; +static sem_t g_tlssem = SEM_INITIALIZER(1); +static tls_dtor_t g_tlsdtor[CONFIG_TLS_TASK_NELEM]; + +/**************************************************************************** + * Public Functions + ****************************************************************************/ + +int task_tls_alloc(tls_dtor_t dtor) +{ + int ret; + int candidate; + + ret = nxsem_wait(&g_tlssem); + + if (ret < 0) + { + return ret; + } + + ret = -EAGAIN; + + for (candidate = 0; candidate < CONFIG_TLS_TASK_NELEM; candidate++) + { + int mask = 1 << candidate; + if ((g_tlsset & mask) == 0) + { + g_tlsset |= mask; + g_tlsdtor[candidate] = dtor; + ret = candidate; + break; + } + } + + nxsem_post(&g_tlssem); + return ret; +} + +/**************************************************************************** + * Name: task_tls_destruct + * + * Description: + * Destruct all TLS data element associated with allocated key + * + * Input Parameters: + * None + * + * Returned Value: + * A set of allocated TLS index + * + ****************************************************************************/ + +void task_tls_destruct(void) Review comment: I will explain what I mean and also make some comments based on your answers. > TLS here mean Task local storage, collide with thread local storage.:( Let's use `task_ls_` and not `task_tls_` so the confusion will be removed. Do you agree? > From the concept, task local storage is very similar to thread local storage, but at the task level. Because I do not have visibility about conditions (when and how) of `task_tls_alloc` call, I can't get a full picture. Based on your example let's assume that it is a time when the library is instantiated for the first time. Then the allocated ID will be shared across the library instances while global data will be stored in Task LS. For example I have 2 different libraries that rely on top of Task LS. The library A attach `dtorA` at index 0 and library B attach `dtorB` at index 1. When one of tasks that use library A exits then `task_tls_destruct` is called and we will call `dtorB` from library A user context point of view. Yes, the `0` will be passed as a parameter, but I'm not sure if such situation is valid from isolation point of view. Even more, if there is a but in library A code and it somehow at some point pass a wrong index to `task_tls_set_value` then `dtorB` will be called with non-zero argument and that value can match with the same value that is allocated by library B users. This potentially can lead to security issues. >> We can have 0 .. 64 as a limit for CONFIG_TLS_TASK_NELEM and use up to uint64_t for index storage. Or add save task pid together with each allocated index so that can be reused in task_tls_destruct to filter destructors to call > > could you explanation more? The code is almost same as the thread local storage, just lift up one level. There are actually two issues here: 1. `CONFIG_TLS_TASK_NELEM` is not limited in Kconfig and there is next code: ``` int task_tls_alloc(tls_dtor_t dtor) { int ret; int candidate; ... for (candidate = 0; candidate < CONFIG_TLS_TASK_NELEM; candidate++) { int mask = 1 << candidate; if ((g_tlsset & mask) == 0) { g_tlsset |= mask; g_tlsdtor[candidate] = dtor; ret = candidate; break; } } ... return ret; } ``` so if `CONFIG_TLS_TASK_NELEM` is more then number of bits that `int` can carry `if ((g_tlsset & mask) == 0)` will start be `true` always and out of boundary array access will occur. Hopefully this will be caught by compiler. 2. Because of lack of use case description I was thinking that each library A user will call `task_tls_alloc()` at the start and will use that allocated index, but maybe it is not a use case (but that is a more like a traditional TLS). So I was thinking that we can store a bit mask to keep tracking of indexes allocated by a task. Again even now I'm not aware about this API usage point of view assuming that it can be called by any task in the system. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
