[bug] Coverity fixes 2025.0 #2890
Conversation
icfaust
requested review from
Alexsandruss,
samir-nasibli and
Alexandr-Solovev
as code owners
|
/intelci: run |
| @@ -589,7 +589,7 @@ algorithmFpType KNNClassificationTrainBatchKernel<algorithmFpType, training::def | |||
|
|
|||
| size_t sumMid = 0; | |||
| size_t i = 0; | |||
| for (; i < sampleCount; ++i) | |||
| for (; i < __KDTREE_MEDIAN_RANDOM_SAMPLE_COUNT; ++i) | |||
There was a problem hiding this comment.
algorithmFpType samples[__KDTREE_MEDIAN_RANDOM_SAMPLE_COUNT + 1];
const size_t sampleCount = sizeof(samples) / sizeof(samples[0]);
should it be __KDTREE_MEDIAN_RANDOM_SAMPLE_COUNT + 1?
There was a problem hiding this comment.
good question, see this line https://github.com/icfaust/oneDAL/blob/dev/coverity-2025_0/cpp/daal/src/algorithms/k_nearest_neighbors/kdtree_knn_classification_train_dense_default_impl.i#L601 when this for loop terminates i == sampleCount (i.e. i == __KDTREE_MEDIAN_RANDOM_SAMPLE_COUNT + 1) so when it is used on that line, it will exceed the array bound.
There was a problem hiding this comment.
good point, agree with you, but probably its a bug, check the line
https://github.com/oneapi-src/oneDAL/pull/2890/files#diff-e5b67a6ac7d03badd31a155779cec86abb98f260008cd311acf5166a0102b123R519
does it make sense to do the same fix here? My point that if we replace sampleCount var with __KDTREE_MEDIAN_RANDOM_SAMPLE_COUNT it makes sense to remove it everywhere, but probably its more correct to fix it as its done at the line 519
There was a problem hiding this comment.
What do you think about initialize sampleCount this way in this PR:
const size_t sampleCount = __KDTREE_MEDIAN_RANDOM_SAMPLE_COUNT + 1
since you are touching this part of code
There was a problem hiding this comment.
done. Still don't understand why they had it as sizeof, I think @Alexandr-Solovev had a good theory.
|
What I'm concerned about is that masterHist array is filled from 0 to sampleCount - 1 inclusive https://github.com/oneapi-src/oneDAL/pull/2890/files#diff-e5b67a6ac7d03badd31a155779cec86abb98f260008cd311acf5166a0102b123R580 If we apply the change from the PR the last value from masterHist array will not be used at all and seems wrong to me, so there is some mistake here. Maybe it is in the line https://github.com/oneapi-src/oneDAL/pull/2890/files#diff-e5b67a6ac7d03badd31a155779cec86abb98f260008cd311acf5166a0102b123L601 ? |
|
/intelci: run |
|
/intelci: run |
Description
Make coverity fixes for 2025.0
Changes proposed in this pull request: