Skip to content

bug: Range loop modification during time-backwards handling [MEDIUM] #40

@CybotTM

Description

@CybotTM

Upstream Reference

Addresses robfig#480 - PR implementing time backwards handling.

Summary

The time-backwards handling code iterates over entries with a range loop while calling Update(), which modifies the underlying slice ordering.

Location

cron.go:277-284

for _, e := range c.entries {
    if !e.Prev.IsZero() && e.Prev.After(now) {
        e.Next = e.Schedule.Next(now)
        c.entries.Update(e)  // Calls heap.Fix which reorders elements
        c.logger.Info("reschedule", ...)
    }
}

Problem

  1. range iterates with original indices
  2. Update()heap.Fix()Swap() reorders elements
  3. After swaps, some entries may be visited twice or skipped

Impact

  • Severity: Medium
  • Only affects rare "time moved backwards" scenario (NTP correction, VM restore)
  • Some entries may miss rescheduling during correction
  • Heap remains structurally valid afterward

Fix Options

  1. Iterate over a copy of the entries
  2. Use index-based iteration accounting for swaps
  3. Collect entries to update first, then update in separate loop
// Option 1: Copy entries
entriesCopy := make([]*Entry, len(c.entries))
copy(entriesCopy, c.entries)
for _, e := range entriesCopy {
    // ... update logic
}

Validation

  • Consensus: Disputed initially, resolved as valid bug with medium severity

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions