Skip to content

Fix invalid memory access when reading colour maps#2974

Merged
bjeurissen merged 1 commit intodevfrom
fix_invalid_mem_access
Aug 21, 2024
Merged

Fix invalid memory access when reading colour maps#2974
bjeurissen merged 1 commit intodevfrom
fix_invalid_mem_access

Conversation

@daljit46
Copy link
Copy Markdown
Member

In 6286158, as part of #2911, ColourMap::maps was converted from a null-terminated C array to a std::vector. However, the loops iterating over this list weren't updated and were still relying on null-termination causing an invalid memory access. This also resulted mrview crashing on MacOS.
@bjeurissen Could you check if this fixes the crash on your system?

@daljit46 daljit46 self-assigned this Aug 21, 2024
@daljit46 daljit46 force-pushed the fix_invalid_mem_access branch from 592b8f5 to e2d81f2 Compare August 21, 2024 10:47
@github-actions
Copy link
Copy Markdown

clang-tidy review says "All clean, LGTM! 👍"

1 similar comment
@github-actions
Copy link
Copy Markdown

clang-tidy review says "All clean, LGTM! 👍"

In 6286158, as part of #2911,
ColourMap::maps was converted from a null-terminated C array to a
std::vector. However, the loops iterating over this list weren't updated
and were still relying on null-termination causing an invalid memory
access. This also resulted mrview crashing on MacOS.
@daljit46 daljit46 force-pushed the fix_invalid_mem_access branch from e2d81f2 to ee8def6 Compare August 21, 2024 10:56
@bjeurissen bjeurissen closed this Aug 21, 2024
@bjeurissen bjeurissen reopened this Aug 21, 2024
Copy link
Copy Markdown
Member

@bjeurissen bjeurissen left a comment

Choose a reason for hiding this comment

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

Works!

@bjeurissen bjeurissen merged commit b830008 into dev Aug 21, 2024
@bjeurissen bjeurissen deleted the fix_invalid_mem_access branch August 21, 2024 11:27
@github-actions
Copy link
Copy Markdown

clang-tidy review says "All clean, LGTM! 👍"

@daljit46
Copy link
Copy Markdown
Member Author

daljit46 commented Aug 23, 2024

Mentioning this for bookkeeping: the changes in this PR have been rebased in dev following discussion in #2859.

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