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

MemorySanitizer: use-of-uninitialized-value in rdtime.h #4865

Open
7 tasks done
fdr400 opened this issue Oct 8, 2024 · 4 comments · May be fixed by #4866
Open
7 tasks done

MemorySanitizer: use-of-uninitialized-value in rdtime.h #4865

fdr400 opened this issue Oct 8, 2024 · 4 comments · May be fixed by #4866

Comments

@fdr400
Copy link

fdr400 commented Oct 8, 2024

Description

librdkafka fails with MemorySanitizer with use-of-unitialized-value in src/rdtime.h.

The error is precisely:

==3601446==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x5555559e8cfd in rd_timeout_init_timespec /home/fdr400/tmp/librdkafka/src/rdtime.h:255:21
    #1 0x5555559e7a76 in rd_kafka_q_serve /home/fdr400/tmp/librdkafka/src/rdkafka_queue.c:543:9
    #2 0x55555564f71e in rd_kafka_thread_main /home/fdr400/tmp/librdkafka/src/rdkafka.c:2155:17
    #3 0x7ffff789c86a in start_thread nptl/pthread_create.c:444:30
    #4 0x7ffff7929c3b in clone3 misc/../sysdeps/unix/sysv/linux/x86_64/clone3.S:78

SUMMARY: MemorySanitizer: use-of-uninitialized-value /home/fdr400/tmp/librdkafka/src/rdtime.h:255:21 in rd_timeout_init_timespec
Exiting

Error is in rd_timeout_init_timespec and in rd_timeout_init_timespec_us.
Their argument tspec is always uninitialized and timespec_get(tspec, TIME_UTC); is not initializing it entirely.

I found the comment about the same issue in zstd code: https://github.com/facebook/zstd/blob/dev/programs/timefn.c#L74

Importance

I am from @userver-framework, we have a Kafka client based on librdkafka, which represents non-blocking and scalable interfaces for producers and consumers.
We have huge number of tests and CI checks both for internal (in company's repository) and external (in Github CI) repositories, under different compilers, build options and sanitizers.
But we cannot enable MSAN sanitizer for Kafka client because of the error.

P.S. I checked that with my fix, our tests works.

P.P.S. Client code: https://github.com/userver-framework/userver/tree/develop/kafka

How to reproduce

I made a repo with instruction and small code example to reproduce the error and check that my fix works.
Repository link: https://github.com/fdr400/rdkafka-producer-msan

Checklist

IMPORTANT: We will close issues where the checklist has not been completed.

Please provide the following information:

  • librdkafka version (release number or git tag): 2.5.3
  • Apache Kafka version: 2.13-3.7.0 (but not used in example)
  • librdkafka client configuration: bootstrap.servers=localhost:9092
  • Operating system: Ubuntu 24.04
  • Logs are here: https://github.com/fdr400/rdkafka-producer-msan/blob/main/README.md
  • Provide broker log excerpts: broker can be not used in the example
  • Critical issue: If it is possible, please review the changes in a few days
@fdr400 fdr400 linked a pull request Oct 8, 2024 that will close this issue
@emasab
Copy link
Contributor

emasab commented Oct 15, 2024

Hi, thanks for the report, the problems seems to be that timespec_get can return an error an in that case the tspec isn't initialized. That should be the problem I think. Could you try if initializing it to zero on error fixes your warning?

@fdr400
Copy link
Author

fdr400 commented Oct 16, 2024

@emasab i tried to do so and it fixes the problem, but i don't think it is better solution, because

  1. it is easy to forget about initialization of tspec before calling the rd_timeout_init_timespec
  2. it may lead to some performance issues caused by unnecessary memset in runtime

Initalization inside rd_timeout_init_timespec also is not a good idea, because, i suppose, it is unexpected from function that it zeroing the input argument. It can return the constructed tspec, but it is not the C style

So, the sanitizer unpoison is enough good solution, because it does nothing in Release and Debug builds

@fdr400
Copy link
Author

fdr400 commented Nov 14, 2024

@emasab

@fdr400
Copy link
Author

fdr400 commented Dec 15, 2024

@emasab could you review again the patch, please

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

Successfully merging a pull request may close this issue.

2 participants