Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dlt-user: fix invalid return in dlt_user_log_write_start_internal #641

Conversation

alexmohr
Copy link
Contributor

dlt_user_log_write_start_internal might return OK when the buffer was not initialized, this can lead to other functions assuming that the buffer was setup correctly and thus access invalid memory.

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, alexander.m.mohr@mercedes-benz.com, Mercedes-Benz Tech Innovation GmbH, imprint

@alexmohr alexmohr marked this pull request as draft June 12, 2024 15:22
dlt_user_log_write_start_internal might return OK when the buffer
was not initalized, this can lead to other functions assuming that
the buffer was setup correctly and thus access invalid memory.

Signed-off-by: Alexander Mohr <alexander.m.mohr@mercedes-benz.com>
@alexmohr alexmohr force-pushed the fix-dlt_user_log_write_start_internal branch from 3087a41 to c6eeabe Compare June 12, 2024 15:38
@alexmohr alexmohr marked this pull request as ready for review June 12, 2024 15:42
@@ -1838,7 +1838,7 @@ DltReturnValue dlt_user_log_write_start_internal(DltContext *handle,
return DLT_RETURN_WRONG_PARAMETER;
} else if (ret == DLT_RETURN_LOGGING_DISABLED) {
log->handle = NULL;
return DLT_RETURN_OK;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be meaning that the behavior of some scenarios when logging disabled should be by-passed here?
I believe there could be some reasons behind doing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be meaning that the behavior of some scenarios when logging disabled should be by-passed here?

I didn't find such a thing but maybe I just missed it. But I'm not sure if it's a good idea to abuse it like this.
If we keep return OK here it should be clearly documented that log->buffer is only initialized when log->handle is not null. We had a crash because an application assumed that the buffer is set up when ok is returned and did not check log->handle.

@minminlittleshrimp minminlittleshrimp dismissed their stale review June 26, 2024 08:51

Not yet having reasonable rca

@alexmohr
Copy link
Contributor Author

Closing this as it has unintended side effects in dlt-qnx-system leading to breaking that service.

@alexmohr alexmohr closed this Jun 28, 2024
@minminlittleshrimp
Copy link
Collaborator

Hello @alexmohr
Could you kindly please go for a brief overview of the side effects?
I am dying to know the reason for closing this and the reason behind the abuse of the return value there.
Many thanks for considering my request.

@alexmohr
Copy link
Contributor Author

Hi @minminlittleshrimp

so the actual crash that we had is due to an internal feature we're developing at the moment (which we will upstream eventually once its mature)
Anyways it contains the following lines of code.

 ret = dlt_user_log_write_start(&handle, &log, loglevel);
 if (ret < DLT_RETURN_OK)
 {
        return ret;
 }

The crash happened when dlt_user_log_write returned true when the buffer in fact was not initialized, so it was logical to me to introduce the change in this pull request.

In dlt-qnx-slogger2-adapter.cppsloggerinfo_callback::sloggerinfo_callback dlt_user_log_write_start is called as well.
If dlt_user_log_write_start returns DLT_RETURN_LOGGING_DISABLED we will exit the sloggerinfo_callback function with -1, which in turn will abort reading messages from the slog as it only continues as long as no error is returned in the callback and therefore dlt-qnx-system stops and no further slogs are read.

It's just a bit confusing that DLT_RETURN_OK means below threshold (there is even a comment in dlt_qnx_system) and DLT_RETURN_TRUE means that the log level is okay and the buffer is set up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants