i.albedo: Fix out of bounds access for albedo histogram#3247
i.albedo: Fix out of bounds access for albedo histogram#3247echoix merged 3 commits intoOSGeo:mainfrom
Conversation
Signed-off-by: Mohan Yelugoti <[email protected]>
wenzeslaus
left a comment
wenzeslaus
left a comment
There was a problem hiding this comment.
Thank you! The fix makes sense and it fits with the initialization loop for (i = 0; i < 100; i++) and with the for (i = 0; i < 100; i++) loop immediately above. Overall, the loop also fits with the two other loops which deal with i_peak2 and 3.
Related, but not preventing this PR from being merged as is: I'm assuming the bb_alb group of functions is returning in interval [0, 1). The bb_alb result is multiplied by 100 and used as an index. Maybe bb_alb < 1 needs to be checked. bb_alb > 0 condition is used, but that actually always skips the bin with index 0, no?
Both of these issues may need to go to errata. This PR fixes the case where a value after the end of the array is used. If that value is 0, it likely fulfills the condition to update bottom3b. The bb_alb issue may cause some values to be ignored if I got that right.
I suggest merging this PR, but if someone can follow up on bb_alb and the possible change of results, that would be great.
Can the main author @YannChemin help? |
|
Line 33 : int histogram[100];Line 333 and 334 : for (i = 100; i > i_peak3; i--) {
if (histogram[i] < bottom3b) {Line 334 first index is Actually it should always have returned an error when using the option histogram (flags c or d), which I do not remember having when using it after creating it. In any case, I recommend a merge of the modification. |
|
This PR is approved and reviewed. Is there anything that should block merging this? I could run a new pass of CI, but there isn't any conflicts in there. |
|
@echoix : Let me close the PR. Thanks everyone for helping me out. |
|
Is there a reason why you chose to close this? The fix is still needed and ready :) |
|
My bad, I thought it was merged. I see that earlier merge in the PR was updating my branch to latest main. Sorry! |
|
I updated to have the latest definitions of the workflows, that would be required to set other checks correctly. |
|
Thanks again @ymdatta for your first contribution here! |
6 participants
This patch fixes out of bounds access while computing Cloud/Snow histogram higher bound during the process of calculating albedo histogram stretch.
Histogram is declared as
int histogram[100]and the maximum index value in the integer array should be 99. While calculating 'Cloud/Snow histogram higher bound', we are iterating starting from index 100, rather than 99. This is undefined behavior in C language and we can't rely on it. This patch fixes this starting index to 99, the maximum possible index for the histogram array.Additional information:
Tool's output after applying the patch:
Signed-off-by: Mohan Yelugoti [email protected]