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

gdcmcharls fix nullptr dereferece #186

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MadVitaliy
Copy link
Contributor

I know that it is external copypasta code. But GCC12 compile rises -Werror=nonnull warning and it makes sense.
According to an exception message, the exception must be thrown if params.thumbnail is nullptr. Moreover, right now the pointer remains nullptr and it is dereferenced in the very next statment. So, I am tent to believe the current version is mistaken and should be fixed.

@seanm
Copy link
Contributor

seanm commented Jan 8, 2025

This cannot be merged into GDCM without checking upstream first. Upstream is : https://github.com/team-charls/charls

  • Does the bug still exist upstream? If so, submit a PR there.
  • Is the bug already fixed upstream? Did they fix it the same way as you?

@seanm
Copy link
Contributor

seanm commented Jan 9, 2025

In fact the CharLS in GDCM is rather old. Would be interesting to see if the newest CharLS works if copied into GDCM...

@MadVitaliy
Copy link
Contributor Author

@seanm, as you already said, the used CharLS sources are rather old. I had tried, but did not find the file I changed in an up-to-date version. I believe it is good to have this fix, unless CharLS is not updated.

@seanm
Copy link
Contributor

seanm commented Jan 9, 2025

It would be better, but of course more work, to update CharLS in GDCM. I'm not sure how backwards-compatible CharLS is generally, if we are lucky GDCM won't even need changes, and it could be a drop-in replacement... feel like trying?

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.

2 participants