dlt-user: Fix crashes in dlt_free during dlt_init #362

alexmohr · 2022-01-19T15:31:14Z

If dlt_free is called while dlt_init is running, the dlt-user library
crashes because the init and free where not thread safe.
This commit introduces a new field dlt_user_initialising
which will prevent entering dlt_free while dlt_init is running.
Correctness is ensured by a unit test

The program was tested solely for our own use cases, which might differ from yours.
Licensed under Mozilla Public License Version 2.0

^{Alexander Mohr, [email protected], Mercedes-Benz Tech Innovation GmbH, imprint}

mbehr1 · 2022-09-24T06:16:46Z

src/lib/dlt_user.c

 static DltUser dlt_user;
-static atomic_bool dlt_user_initialised = false;
+static enum InitState dlt_user_init_state = INIT_UNITIALIZED;


shouldn't this be "static volatile enum ..." as you're later using atomic_compare_exchange_strong with it?

Yes good point, added it 👍

This needs to be:
static _Atomic enum InitState dlt_user_init_state = INIT_UNITIALIZED;
and also:
static _Atomic int dlt_user_freeing = 0;

otherwise it does not compile with clang anymore.

michael-methner · 2022-10-20T11:59:55Z

src/lib/dlt_user.c

        return DLT_RETURN_OK;
+    }


Hello @alexmohr ,
i think we still have a race condition here:

Thead 1: dlt_init() is called

Thread 2: dlt_init() is called, immediately returns DLT_OK as atomic operation fails at this point here

Thread 1: continues initialization and fails due to whatever reason.
so for thread 2 everything would have been fine but DLT library is still not initialzed.

Would a mutex make sense here to block thread 2 until thread 1 has finished the initialization?

Hi @michael-methner the described scenario could leave the library in an initialized state.
Adding a mutex here sounds like a good idea. I'll add it later

@michael-methner It's done in case you did not notice it yet :)

Hello @alexmohr ,
sorry for the delay and thank you very much for the rework. I was quite busy the last weeks. I need some time to carefully review this as it is related to race conditions ;). I hope I can manage before end of this week.

One first question: Would it make sense to use the same mutex in dlt_free() as well to avoid race conditions between dlt_init() and dlt_free().

I don't think it's necessary as the atomic variable already should make sure that the free method is not running in parallel. In contrast to init, multiple tries of free do not make that much sense.

Hello @alexmohr ,
thanks for your comment. I agree with your statement that the mutex is not required in dlt_free(). It should be fine as the state is checked with the macro DLT_USER_INITALIZED().

I think the PR good to merge after the typos are fixed and the two static variables are set to "_Atomic".

And sorry, that you had to wait that long for the feedback.

Thanks for the feedback. Both variables are changed to _Atomic and it is building locally with clang 10 and gcc 10.3

lvklevankhanh · 2022-12-02T08:41:14Z

src/lib/dlt_user.c

@@ -90,8 +90,17 @@
 #   define DLT_LOG_FATAL_RESET_TRAP(LOGLEVEL)
 #endif /* DLT_FATAL_LOG_RESET_ENABLE */

+enum InitState {


Nice to have if the name of enum should be DltInitState

lvklevankhanh · 2022-12-02T08:42:51Z

src/lib/dlt_user.c

+    enum InitState expectedInitState = INIT_UNITIALIZED;
+    if (!(atomic_compare_exchange_strong(&dlt_user_init_state, &expectedInitState, INIT_IN_PROGRESS))) {
+        /* skip init if we're initialized, otherwise try init (again) */
+        if (dlt_user_init_state == INIT_DONE)


Nice to have: the condition should be "if (DLT_USER_INITALIZED)"

lvklevankhanh · 2022-12-02T08:44:08Z

src/lib/dlt_user.c

@@ -1735,7 +1762,7 @@ int dlt_set_resend_timeout_atexit(uint32_t timeout_in_milliseconds)
    if (g_dlt_is_child)
        return DLT_RETURN_ERROR;

-    if (dlt_user_initialised == 0)
+    if (DLT_USER_INITALIZED == 0)


Nice to have: the condition should be "if (!DLT_USER_INITALIZED)"

lvklevankhanh · 2022-12-02T08:45:10Z

src/lib/dlt_user.c

    }

    /* prepare for fork() call */
    pthread_atfork(NULL, NULL, &dlt_fork_child_fork_handler);

+    /* set success before jump label because in case we used the jump INIT_DON


Nice to have: "INIT_DONE" instead "INIT_DON"

alexmohr · 2022-12-03T00:04:19Z

@lvklevankhanh thanks for your comments I'm going to fix as soon as I'm back home. It will take a couple of weeks though. Just saying so you don't close the PR due to inactivity :)

michael-methner · 2023-01-04T14:33:30Z

src/lib/dlt_user.c

@@ -90,8 +90,17 @@
 #   define DLT_LOG_FATAL_RESET_TRAP(LOGLEVEL)
 #endif /* DLT_FATAL_LOG_RESET_ENABLE */

+enum InitState {
+    INIT_UNITIALIZED,


typo: INIT_UNINTIALIZED

michael-methner · 2023-01-04T14:43:33Z

src/lib/dlt_user.c

-static atomic_bool dlt_user_initialised = false;
+static volatile enum InitState dlt_user_init_state = INIT_UNITIALIZED;
+static pthread_mutex_t dlt_user_init_mtx = PTHREAD_MUTEX_INITIALIZER;
+#define DLT_USER_INITALIZED (dlt_user_init_state == INIT_DONE)


typo: DLT_USER_INITIALIZED

If dlt_free is called while dlt_init is running, the dlt-user library crashes because the init and free where not thread safe. This commit introduces a new field dlt_user_initialising which will prevent entering dlt_free while dlt_init is running. Correctness is ensured by a unit test Signed-off-by: Alexander Mohr <[email protected]>

mbehr1 reviewed Sep 24, 2022

View reviewed changes

alexmohr force-pushed the fix_init_free_error branch from 2be5223 to c608cc7 Compare September 27, 2022 11:23

michael-methner reviewed Nov 2, 2022

View reviewed changes

alexmohr force-pushed the fix_init_free_error branch 2 times, most recently from b8b0992 to bcd9d39 Compare November 10, 2022 11:55

lvklevankhanh reviewed Dec 2, 2022

View reviewed changes

michael-methner reviewed Jan 4, 2023

View reviewed changes

alexmohr force-pushed the fix_init_free_error branch from bcd9d39 to 203d788 Compare January 9, 2023 10:45

michael-methner merged commit 2224cdd into COVESA:master Jan 11, 2023

minminlittleshrimp mentioned this pull request Jun 6, 2023

dlt_unit_test: Fix and improve quality of unit tests #494

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dlt-user: Fix crashes in dlt_free during dlt_init #362

dlt-user: Fix crashes in dlt_free during dlt_init #362

alexmohr commented Jan 19, 2022 •

edited

Loading

mbehr1 Sep 24, 2022

alexmohr Sep 27, 2022

michael-methner Jan 4, 2023

michael-methner Oct 20, 2022

alexmohr Nov 2, 2022

alexmohr Nov 21, 2022

michael-methner Nov 30, 2022

alexmohr Nov 30, 2022

michael-methner Jan 4, 2023

alexmohr Jan 9, 2023

lvklevankhanh Dec 2, 2022

lvklevankhanh Dec 2, 2022

lvklevankhanh Dec 2, 2022

lvklevankhanh Dec 2, 2022

alexmohr commented Dec 3, 2022

michael-methner Jan 4, 2023

michael-methner Jan 4, 2023

dlt-user: Fix crashes in dlt_free during dlt_init #362

dlt-user: Fix crashes in dlt_free during dlt_init #362

Conversation

alexmohr commented Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexmohr commented Dec 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexmohr commented Jan 19, 2022 •

edited

Loading