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

i.ortho.photo: Fix uninitialized variable and potential buffer overflow #4093

Merged
merged 9 commits into from
Aug 2, 2024

Conversation

ShubhamDesai
Copy link
Contributor

This issue was identified by coverity scan (CID: 1415632).
Initially cppcheck was used on imagery folder and all uninitialized variables were resolved but this issue was missed by cppcheck.

Changes Made:
Initialized the tosystem array with an empty string (char tosystem[99] = "";).

@github-actions github-actions bot added C Related code is in C module imagery labels Jul 23, 2024
Copy link
Contributor

@lbartoletti lbartoletti left a comment

Choose a reason for hiding this comment

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

Two notes,

99 should be in a #DEFINE constant as used many times in the file.

tosystem is later used with strcpy, even if it's "safe" (< 99), we could use strncpy

@echoix
Copy link
Member

echoix commented Jul 24, 2024

@ShubhamDesai tell me if you don't want to apply the suggestions in this same PR (I think is reasonable). If it is the case, I'll make the PR mergeable.

Thanks @lbartoletti for reviewing! I wasn't completely sure about the pattern to use when initializing char arrays, and that I wasn't missing something in the context around.
His other unmerged PRs is the same, I don't feel confident enough to check it by myself, if you'd like to take a look too

@ShubhamDesai
Copy link
Contributor Author

@ShubhamDesai tell me if you don't want to apply the suggestions in this same PR (I think is reasonable). If it is the case, I'll make the PR mergeable.

Thanks @lbartoletti for reviewing! I wasn't completely sure about the pattern to use when initializing char arrays, and that I wasn't missing something in the context around. His other unmerged PRs is the same, I don't feel confident enough to check it by myself, if you'd like to take a look too

I would edit in the same PR so that it would be easily reviewed

@ShubhamDesai
Copy link
Contributor Author

Two notes,

99 should be in a #DEFINE constant as used many times in the file.

tosystem is later used with strcpy, even if it's "safe" (< 99), we could use strncpy

I have done the changes. Could you please review it once

Copy link
Member

@echoix echoix left a comment

Choose a reason for hiding this comment

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

@lbartoletti Do you mind a quick double check here? I don't see anything wrong. The same pattern is applied averywhere (strcpy -> G_strlcpy), except for group.name (strncpy -> G_strlcpy).

Also, does @nilason's comment here #4087 (comment) about a possible string truncation apply here too?

Other than that, I would approve it

imagery/i.ortho.photo/i.ortho.photo/menu.c Outdated Show resolved Hide resolved
imagery/i.ortho.photo/i.ortho.photo/menu.c Outdated Show resolved Hide resolved
imagery/i.ortho.photo/i.ortho.photo/menu.c Outdated Show resolved Hide resolved
imagery/i.ortho.photo/i.ortho.photo/menu.c Outdated Show resolved Hide resolved
imagery/i.ortho.photo/i.ortho.photo/menu.c Outdated Show resolved Hide resolved
ShubhamDesai and others added 5 commits August 1, 2024 20:01
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Contributor

@nilason nilason left a comment

Choose a reason for hiding this comment

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

Thanks!

@nilason nilason changed the title i.ortho.photo: Fix uninitialized variable issue in menu.c i.ortho.photo: Fix uninitialized variable and potential buffer overflow Aug 2, 2024
@nilason nilason added this to the 8.5.0 milestone Aug 2, 2024
@nilason nilason merged commit e671a43 into OSGeo:main Aug 2, 2024
27 checks passed
Mahesh1998 pushed a commit to Mahesh1998/grass that referenced this pull request Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C Related code is in C imagery module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants