frollmedianFast: avoid reading uninitialised array #7589
Conversation
The 'n' array is only initialised if 'even' is true, so skip the comparisons otherwise. Detected by checking with --use-valgrind or performing a frollmedian() with an odd window size under R -d valgrind. Fixes: #7546
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7589 +/- ##
==========================================
- Coverage 99.02% 99.02% -0.01%
==========================================
Files 87 87
Lines 16803 16804 +1
==========================================
Hits 16640 16640
- Misses 163 164 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
No obvious timing issues in HEAD=frollmedian-even-undefined Generated via commit f11e0ff Download link for the artifact containing the test results: ↓ atime-results.zip
|
|
LGTM, I'll let Jan give final approval. Should we add a new test? codecov says we lost a line. |
ben-schwen
left a comment
ben-schwen
left a comment
There was a problem hiding this comment.
LGTM.
I understand the hot path argument, but given that we just had a bug about access to an uninitialized array (in only 1 of 2 paths), I would be in favor of trying to unify both paths in favor lowering the maintenance burden.
| n[B] = tail; | ||
| if (even) { | ||
| if (n[A]!=tail && m[A] == n[A]) { | ||
| n[A] = tail; |
There was a problem hiding this comment.
I'm struggling to devise a test that would reach this line. The closest I've got was n[A] = m[A] = tail = 4 for frollmedian(c(1:4, rep(1,3)), 4).
3 participants
The
narray is only initialised ifevenis true, so skip the comparisons otherwise. Detected by checking with--use-valgrindor performing afrollmedian()with an odd window size underR -d valgrind.Fixes: #7546