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

Use std::unique_ptr rather than raw pointer #3179

Merged
merged 7 commits into from
Feb 20, 2025

Conversation

kevinbackhouse
Copy link
Collaborator

This is a follow-up to #3174. Switching to std::unique_ptr means that we cannot accidentally introduce the same bug again. This PR doesn't change the behavior of the copy constructor, so when you clone a TiffComponent, these fields don't get copied. I'm not sure if that's actually the correct behavior, but it's the way it was before, so I think it's safer not to change it for the purposes of a security fix. If we decide to change the cloning behavior in the future then it might make sense to replace these unique_ptrs with shared_ptrs.

@kevinbackhouse
Copy link
Collaborator Author

@Mergifyio backport 0.28.x

@kevinbackhouse kevinbackhouse requested a review from neheb February 19, 2025 22:20
Copy link
Contributor

mergify bot commented Feb 19, 2025

backport 0.28.x

✅ Backports have been created

@kevinbackhouse kevinbackhouse linked an issue Feb 19, 2025 that may be closed by this pull request
@neheb
Copy link
Collaborator

neheb commented Feb 19, 2025

Overall this seems fine. It would be better to eventually move to shared_ptr and get rid of all of these get() and release() calls. They just look like an awkward solution to unique_ptr having deleted copy operations.

@kevinbackhouse kevinbackhouse force-pushed the tiffcomposite-unique_ptr branch from 05f7fd0 to 3be906d Compare February 19, 2025 22:49
@neheb
Copy link
Collaborator

neheb commented Feb 19, 2025

You need to run clang-format -i src/tiffcomposite_int.hpp

git clang-format is not enough here.

@kevinbackhouse
Copy link
Collaborator Author

You need to run clang-format -i src/tiffcomposite_int.hpp

git clang-format is not enough here.

Thanks, I'm on my phone right now, but I'll fix this and your other suggestions later today.

I did try running clang-format but it changed other unrelated parts of this file, so I thought I would try without it first. I'll have to check if it's related to the version of clang-format that I used.

@kevinbackhouse kevinbackhouse merged commit 0098d71 into Exiv2:main Feb 20, 2025
60 checks passed
@kevinbackhouse kevinbackhouse deleted the tiffcomposite-unique_ptr branch February 20, 2025 20:38
@kmilos
Copy link
Collaborator

kmilos commented Feb 20, 2025

@kevinbackhouse @neheb Just checking - this is strictly not needed to be backported for 0.28.5?

@kevinbackhouse
Copy link
Collaborator Author

@kevinbackhouse @neheb Just checking - this is strictly not needed to be backported for 0.28.5?

That's true. It really depends on how many more releases we're going to do on the 0.28.x branch. The main benefit of this change is that we'll get a compile error if we ever accidentally introduce the same bug again. So if we think that 0.28.x is going to get frozen soon (and we start a new 0.29.x series) then we don't need to backport this.

@kmilos
Copy link
Collaborator

kmilos commented Feb 21, 2025

Although the main codebase diverged sufficiently indeed, I feel it might be just a bit too early to wind down 0.28.x straight away (Debian 13 and RHEL 10 shipping 0.28.x are not out yet, but just about to; C++20 support across compilers might still be a bit spotty to spring on people, etc.), so it's not inconceivable to have one or two more patch releases...

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 this pull request may close these issues.

Use after free error when calling API Exiv2::TiffParser::encode
3 participants