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

Return of address of stack memory associated with local variable #214

Closed
monwarez opened this issue May 1, 2022 · 1 comment · Fixed by #217
Closed

Return of address of stack memory associated with local variable #214

monwarez opened this issue May 1, 2022 · 1 comment · Fixed by #217

Comments

@monwarez
Copy link
Contributor

monwarez commented May 1, 2022

virtual const char *what() const throw() override {
std::string s = " ";
auto ts = [](size_t x){ return std::to_string(x); };
std::string msg = std::string("Target buffer size is smaller than source: [source size, buffer size]")
+s+ ts(source_size) +s+ ts(target_size);
return msg.c_str();

This code trigger -Wreturn-stack-address since this is not a known at compile time string, and thus the content of the string will be use after free by the caller. Ideally if the exception would just return a string for each implementation, there will be no issue.

@monwarez
Copy link
Contributor Author

monwarez commented May 2, 2022

I think that if the string msg is stored (like in some other exception classes) as a member then this should be good. But this will certainly break ABI.

szszszsz added a commit that referenced this issue Nov 15, 2022
Use static string object for keeping the c_str message for the caller.
Strings collection used as an alternative to memory leaks done via strdup().
To add, TargetBufferSmallerThanSource Exception should never happen in a correctly written library client, as this completely depends on the implementation and not on the communication with the device

Downside: if missed and when occurring too often, the memory taken by the exception messages can take too much memory.
Potential improvement: replace std::vector with a kind of a ring buffer, or add simple wrapping logic over it.

This should avoid breaking ABI by using static memory. Note: there might be multiple exceptions' strings collections (for each compilation unit including this header).
Correct that for libnitrokey 4, by keeping string as a member of the exception.

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

Successfully merging a pull request may close this issue.

2 participants