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

Undefined behaviour when copying dynamic_format_arg_store #2653

Closed
lucpelletier opened this issue Dec 14, 2021 · 8 comments
Closed

Undefined behaviour when copying dynamic_format_arg_store #2653

lucpelletier opened this issue Dec 14, 2021 · 8 comments

Comments

@lucpelletier
Copy link
Contributor

The copy constructor of dynamic_format_arg_store completely ignores the dynamic_args_ private member variable. This can unexpectedly cause undefined behaviour when a dynamic_format_arg_store instance is copied, since anything stored in dynamic_args_ will not get copied over. See the following for an example:

https://godbolt.org/z/8n7rnM55E

I can think of a couple of ways of fixing this problem, and I'd be happy to provide a PR with a fix, but I first wanted to get input from library maintainers on what would be the best solution.

@vitaut
Copy link
Contributor

vitaut commented Dec 14, 2021

The easiest solution is to mark the copy ctor as deleted. A PR would be welcome.

@lucpelletier
Copy link
Contributor Author

#2432 added the copy constructor and seemed to have a use case for it. Are you saying this is no longer required and the copy ctor should now be removed?

@vitaut
Copy link
Contributor

vitaut commented Dec 16, 2021

I think we should disable the copy ctor because it's currently broken. If someone has a use case they can contribute a fix later.

@vitaut
Copy link
Contributor

vitaut commented Dec 18, 2021

Disable the broken copy ctor in be51ee1. I don't think there is much reason to copy dynamic_format_arg_store at all, it's better to copy the actual arguments and construct the argument list only before a formatting function call.

@vitaut vitaut closed this as completed Dec 18, 2021
@vitaut
Copy link
Contributor

vitaut commented Dec 18, 2021

Thanks for reporting.

@spyridon97
Copy link

spyridon97 commented Mar 22, 2022

@vitaut The actual arguments cannot be copied from an dynamic_format_arg_store (there is no public fuction for such functionality) so i believe the copy constructor is the way to go.

I did not copy the dynamic_args_ inside dynamic_format_arg_store because dynamic_arg_list also does not have a copy constructor. I don't think it's possible to add one, am i right?

@spyridon97
Copy link

@vitaut Any suggested solution?

@vitaut
Copy link
Contributor

vitaut commented Mar 26, 2022

Any suggested solution?

I see you've submitted a PR (#2832) so let's discuss there.

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

3 participants