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

Fix ODR violation in ThemisPP #576

Merged
merged 1 commit into from
Jan 27, 2020
Merged

Conversation

ilammy
Copy link
Collaborator

@ilammy ilammy commented Jan 23, 2020

All functions defined in header files must be marked inline so that the compiler does proper deduplication and does not violate “one definition rule” (ODR). Otherwise (you guessed it, right?)
undefined behavior happens.

Thanks goes go clang-analyzer whose warnings we diligently ignore. We've been burnt by this in the past (see #540, and there were customers with broken builds because of this). I never learn because C++ is such a monster that I easily forget something about it all the time.

This is new symmetric key generation API that is pending for release in Themis 0.13.

Checklist

  • Change is covered by automated tests
  • The coding guidelines are followed
  • Public API has proper documentation

All functions defined in header files should be marked "inline"
so that the compiler does proper deduplication and does not violate
"one definition rule" (ODR). Otherwise (you guessed it, right?)
undefined behavior happens.

Thanks goes go "clang-analyzer" whose warnings we diligently ignore.
We've been burnt by this in the past and I never learn because C++ is
such a monster that I easily forget something about it all the time.
@ilammy ilammy added the W-ThemisPP ⚔️ Wrapper: ThemisPP, C++ API label Jan 23, 2020
@Lagovas
Copy link
Collaborator

Lagovas commented Jan 23, 2020

where we define it in several places? or it conflicts with functions from C library?

@ilammy
Copy link
Collaborator Author

ilammy commented Jan 23, 2020

where we define it in several places?

We don‘t but the users will. If ThemisPP is used in multiple translation units (say, A.cpp and B.cpp) both of which #include <themispp/secure_keygen.hpp> then both of them will “define” this function — ThemisPP being header-only library provides a definition, not just declaration. If it’s not marked inline then this might lead to a linker error caused by multiple conflicting definitions of a symbol. inline can be used to tell the linker to ignore duplicates.

Copy link
Contributor

@vixentael vixentael left a comment

Choose a reason for hiding this comment

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

good job! some of our users caught this error

@ilammy ilammy merged commit 077561d into cossacklabs:master Jan 27, 2020
@ilammy ilammy deleted the fix-odr-cpp branch January 27, 2020 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
W-ThemisPP ⚔️ Wrapper: ThemisPP, C++ API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants