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

log_msg constructor doesn't initialize all fields #888

Closed
curiouserrandy opened this issue Oct 29, 2018 · 5 comments
Closed

log_msg constructor doesn't initialize all fields #888

curiouserrandy opened this issue Oct 29, 2018 · 5 comments

Comments

@curiouserrandy
Copy link

The constructors for spdlog::details::log_msg do not initialize all fields of the struct. This can result in failures in consumer code that automatically scans for uninitialized memory accesses. It would be useful for that class to have an explicit default for all fields.

@gabime
Copy link
Owner

gabime commented Oct 29, 2018

Do you have a concrete example of such scan failure ? Valgrind never complained..

gabime added a commit that referenced this issue Oct 29, 2018
@gabime
Copy link
Owner

gabime commented Oct 30, 2018

Fixed. Remved the default ctor of log_msg

@curiouserrandy
Copy link
Author

The test it was found in was MainCommonTest.ConstructDestructLogger in https://github.com/envoyproxy/envoy/blob/master/test/exe/main_common_test.cc, but it occurred in Google internal testing with Memory Sanitizer, and thus I can't say for sure that valgrind would have caught it (though it seems likely).

I'm about to add a change to that test (changing the constructor from the default one over to the one with arguments, making the variable static for zero-initializing before construction) just to get around this problem. That PR is envoyproxy/envoy#4884, so you may need to click back before whatever commit that lands in if it lands before you look.

Thanks much for the quick fix!

@gabime
Copy link
Owner

gabime commented Oct 30, 2018

I believe that no unintialized fields exists when using this (fixed) ctor, so no need to make it static. Moreover, the envoy code probably doesnt use static log_msg, so it’s better to test the same usage as in envoy.

@curiouserrandy
Copy link
Author

Yep. We'll try to switch it back the next time envoy upgrades its dependent spdlog.

@gabime gabime closed this as completed Oct 30, 2018
bachittle pushed a commit to bachittle/spdlog that referenced this issue Dec 22, 2022
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

No branches or pull requests

2 participants