-
-
Notifications
You must be signed in to change notification settings - Fork 307
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.albedo: Fix out of bounds access for albedo histogram #3247
Conversation
Signed-off-by: Mohan Yelugoti <ymdatta.work@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |
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 ymdatta.work@gmail.com