Skip to content

Added check for integer overflow in get_num_images #1397

Merged
rouault merged 28 commits intouclouvain:masterfrom
Eharve14:master
Jan 15, 2022
Merged

Added check for integer overflow in get_num_images #1397
rouault merged 28 commits intouclouvain:masterfrom
Eharve14:master

Conversation

@Eharve14
Copy link
Copy Markdown
Contributor

As discussed in pull request 1396, added a check for integer overflow.
Change list:
Defined num_images as unsigned int
Moved the if statement to check for an empty directory to the beginning of the read directory section
Added a check to see if num images rolls back to zero when incrementing.

Eharve14 and others added 24 commits January 13, 2022 00:53
Removed automatically generated settings.json
…d for conformity, relocated check for num images for exicution before allocation and image loading
This reverts commit dbe64d6.
… unsigned for conformity, relocated check for num images for exicution before allocation and image loading"

This reverts commit ab6c7c7.
Added overflow protection in get_num_images function, redefined num_images to unsigned int in compress and decompress to match dump
…t exicution of allocation and strcpy if there are no images.
Comment thread src/bin/jp2/opj_decompress.c
Comment thread src/bin/jp2/opj_dump.c
Comment thread src/bin/jp2/opj_compress.c Outdated
@Eharve14
Copy link
Copy Markdown
Contributor Author

@rouault I made the requested revisions. I was under the impression that unsigned overflow was always defined behavior. Thank you for the feedback.

Comment thread src/bin/jp2/opj_compress.c
Comment thread src/bin/jp2/opj_decompress.c
Comment thread src/bin/jp2/opj_dump.c
@Eharve14
Copy link
Copy Markdown
Contributor Author

Added the return statement after the overflow check. I don't know how I managed to mess up copy paste.

@rouault rouault merged commit 6e4588f into uclouvain:master Jan 15, 2022
@kirotawa
Copy link
Copy Markdown

kirotawa commented Mar 11, 2022

hey there,

Are these fixes anyhow related with CVE-2021-29338 ? Should it have another CVE assigned to these fixes/commits in this merge?

Thanks!

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.

3 participants