Skip to content

i.atcorr: Fix out-of-bounds array access in trunca computations#3909

Open
ShubhamDesai wants to merge 6 commits intoOSGeo:mainfrom
ShubhamDesai:issue_1
Open

i.atcorr: Fix out-of-bounds array access in trunca computations#3909
ShubhamDesai wants to merge 6 commits intoOSGeo:mainfrom
ShubhamDesai:issue_1

Conversation

@ShubhamDesai
Copy link
Contributor

@ShubhamDesai ShubhamDesai commented Jun 19, 2024

This pull request fixes several instances of out-of-bounds array access in the computations.cpp file within the i.atcorr module. Specifically, it addresses issues where negative indices were being used to access arrays, which could lead to undefined behavior and potential crashes.

Changes Made

  • Added a condition to ensure kk is not negative before using it as an array index.
    if (kk < 0) kk = 0;

Issues Resolved

  1. Error : Array sixs_trunc.pha[83] accessed at index -1, which is out of bounds.
    Location : imagery/i.atcorr/computations.cpp:116:39
    Details : (double)((log10(sixs_trunc.pha[kk]) - log10(sixs_trunc.pha[k])) /

  2. Error : Assuming condition is false
    Location : imagery/i.atcorr/computations.cpp:110:20
    Details : if (rmu[i] > 0.94)

  3. Error : Assignment 'kk=i-1', assigned value is -1
    Location : imagery/i.atcorr/computations.cpp:112:16
    Details : kk = i - 1;

  4. Error : Negative array index
    Location : imagery/i.atcorr/computations.cpp:116:39
    Details : (double)((log10(sixs_trunc.pha[kk]) - log10(sixs_trunc.pha[k])) /

  5. Error : Array rmu[83] accessed at index -1, which is out of bounds.
    Location : imagery/i.atcorr/computations.cpp:117:27
    Details : (acos(rmu[kk]) - acos(rmu[k])));

  6. Error : Assuming condition is false
    Location : imagery/i.atcorr/computations.cpp:110:20
    Details : if (rmu[i] > 0.94)

  7. Error : Assignment 'kk=i-1', assigned value is -1
    Location : imagery/i.atcorr/computations.cpp:112:16
    Details : kk = i - 1;

  8. Error : Negative array index
    Location : imagery/i.atcorr/computations.cpp:117:27
    Details : (acos(rmu[kk]) - acos(rmu[k])));

  9. Error : Array sixs_trunc.pha[83] accessed at index -1, which is out of bounds.
    Location : imagery/i.atcorr/computations.cpp:118:46
    Details : double x1 = (double)(log10(sixs_trunc.pha[kk]));

  10. Error : Assuming condition is false
    Location : imagery/i.atcorr/computations.cpp:110:20
    Details : if (rmu[i] > 0.94)

  11. Error : Assignment 'kk=i-1', assigned value is -1
    Location : imagery/i.atcorr/computations.cpp:112:16
    Details : kk = i - 1;

  12. Error : Negative array index
    Location : imagery/i.atcorr/computations.cpp:118:46
    Details : double x1 = (double)(log10(sixs_trunc.pha[kk]));

  13. Error : Array rmu[83] accessed at index -1, which is out of bounds.
    Location : imagery/i.atcorr/computations.cpp:119:33
    Details : double x2 = (double)acos(rmu[kk]);

  14. Error : Assuming condition is false
    Location : imagery/i.atcorr/computations.cpp:110:20
    Details : if (rmu[i] > 0.94)

  15. Error : Assignment 'kk=i-1', assigned value is -1
    Location : imagery/i.atcorr/computations.cpp:112:16
    Details : kk = i - 1;

  16. Error : Negative array index
    Location : imagery/i.atcorr/computations.cpp:119:33
    Details : double x2 = (double)acos(rmu[kk]);

@github-actions github-actions bot added C++ Related code is in C++ module imagery labels Jun 20, 2024
ShubhamDesai and others added 3 commits June 19, 2024 22:52
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@nilason nilason requested a review from YannChemin July 4, 2024 21:52
@nilason nilason added this to the 8.5.0 milestone Jul 4, 2024
@nilason
Copy link
Contributor

nilason commented Jul 9, 2024

@YannChemin Would this minor change to i.atcorr lead to any unintended consequences?

@wenzeslaus wenzeslaus changed the title lib: Fix out-of-bounds array access in i.atcorr/computations.cpp i.atcorr: Fix out-of-bounds array access in trunca computations Aug 8, 2024
@echoix echoix requested review from YannChemin and removed request for YannChemin September 3, 2024 22:31
@wenzeslaus
Copy link
Member

@ShubhamDesai Check this suggestion from ChatGPT: https://chatgpt.com/share/66f18923-0348-8009-bf2a-e415c5ae9a89

With or without that suggestion, this PS seems like it will require a regression test to make sure we are not breaking this. Can you write one in a separate PR? Either pytest style you used before for g.version (but with use of, e.g., r.univar to get raster values) or it might be easier to follow the unittest style with existing dataset like e.g., https://github.com/OSGeo/grass/blob/main/vector/v.surf.rst/testsuite/test_vsurfrst.py (check the assertVectorFitsUnivar calls). We can discuss in more detail when we meet.

@ShubhamDesai
Copy link
Contributor Author

@ShubhamDesai Check this suggestion from ChatGPT: https://chatgpt.com/share/66f18923-0348-8009-bf2a-e415c5ae9a89

With or without that suggestion, this PS seems like it will require a regression test to make sure we are not breaking this. Can you write one in a separate PR? Either pytest style you used before for g.version (but with use of, e.g., r.univar to get raster values) or it might be easier to follow the unittest style with existing dataset like e.g., https://github.com/OSGeo/grass/blob/main/vector/v.surf.rst/testsuite/test_vsurfrst.py (check the assertVectorFitsUnivar calls). We can discuss in more detail when we meet.

Sure I will write the regression test for it.
I will refer the documentation first and other closely related modules testcases

@nilason
Copy link
Contributor

nilason commented Apr 28, 2025

I'm not sure what tool reports this as out-of-bounds array access. However, kk can't possibly be negative:

int kk = 0;
for (i = 0; i < 83; i++) {
if (rmu[i] > 0.94)
break;
kk = i;
}

make it clear it must be between 0 and 82.

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

Status: In Progress

Development

Successfully merging this pull request may close these issues.

4 participants