Skip to content

fix(util): avoid panic when parsing a bare "s" duration - #2633

Open
BOURBONCASK wants to merge 2 commits into
eclipse-zenoh:mainfrom
BOURBONCASK:fix/time-range-parse-duration-s-panic
Open

fix(util): avoid panic when parsing a bare "s" duration#2633
BOURBONCASK wants to merge 2 commits into
eclipse-zenoh:mainfrom
BOURBONCASK:fix/time-range-parse-duration-s-panic

Conversation

@BOURBONCASK

@BOURBONCASK BOURBONCASK commented Jun 4, 2026

Copy link
Copy Markdown

Description

What does this PR do?

parse_duration (in zenoh-util's time DSL) panics on the input "s".

It reads the duration's trailing byte to detect the unit, and for s it reads a second byte to disambiguate s (seconds) from ms (milliseconds):

let mut it = s.bytes().enumerate().rev();
match it.next().unwrap() {                    // guarded by the leading is_empty() check
    ...
    (_, b's') => match it.next().unwrap() {   // <-- second next(): NOT guarded
        (i, b'm') => ... // "ms"
        (i, _)    => ... // "s"
    },
    ...
}

The leading is_empty() guard only covers the first next(). The input "s" (a unit with no numeric part) exhausts the iterator, so the second unwrap() panics with called Option::unwrap() on a None value. Every other unit-only input ("u", "m", "h", "d", "w") only calls next() once and already returns an Err; "s" is the only unit that double-consumes.

This change handles the exhausted-iterator case like the other unit-only inputs and returns a parse error instead of panicking. It also extends test_parse_duration with all unit-only inputs and end-to-end TimeExpr/TimeRange cases.

Minimal repro on main:

use zenoh_util::time_range::{TimeExpr, TimeRange};

let _ = "now(s)".parse::<TimeExpr>();      // panics
let _ = "[now(s)..]".parse::<TimeRange>(); // panics

Why is this change needed?

parse_duration is reachable from the public time DSL (TimeExpr / TimeRange), which backs the standardized _time selector parameter (ZenohParameters::time_range()). That value comes from the querier's selector, so a malformed _time (e.g. _time=[now(s)..]) is untrusted input that can panic the parsing side — both when sending a get() (ConsolidationMode::Auto reads time_range()) and on a queryable/storage that inspects _time.

The intent is clearly to fail gracefully: time_range() returns Option<ZResult<TimeRange>>, i.e. malformed input is supposed to yield an Err, not a panic. This follows the same "reject malformed input instead of panicking" direction as recent parsing fixes.

Related Issues

N/A — found via review of the time-DSL parsing path.


🏷️ Label-Based Checklist

Based on the labels applied to this PR, please complete these additional requirements:

Labels: bug

🐛 Bug Fix Requirements

Since this PR is labeled as a bug fix, please ensure:

  • Root cause documented - Explain what caused the bug in the PR description
  • Reproduction test added - Test that fails on main branch without the fix
  • Test passes with fix - The reproduction test passes with your changes
  • Regression prevention - Test will catch if this bug reoccurs in the future
  • Fix is minimal - Changes are focused only on fixing the bug
  • Related bugs checked - Verified no similar bugs exist in related code

Why this matters: Bugs without tests often reoccur.

Instructions:

  1. Check off items as you complete them (change - [ ] to - [x])
  2. The PR checklist CI will verify these are completed

This checklist updates automatically when labels change, but preserves your checked boxes.

`parse_duration` reads the duration's trailing byte to detect its unit.
For the `s` unit it then reads a *second* byte to disambiguate `s`
(seconds) from `ms` (milliseconds), using `it.next().unwrap()`. The
leading `is_empty()` guard only covers the first `next()`, so the input
`"s"` (a unit with no numeric part) exhausts the iterator and the second
`unwrap()` panics with `called Option::unwrap() on a None value`.

Every other unit-only input (`"u"`, `"m"`, `"h"`, ...) already returns an
error; `"s"` is the only one that double-consumes. This is reachable from
the public time DSL via `TimeExpr`/`TimeRange` parsing, e.g. a selector
parameter `_time=[now(s)..]`, so untrusted input can trigger the panic.

Handle the exhausted-iterator case like the other unit-only inputs and
return a parse error instead of panicking. Extend `test_parse_duration`
with unit-only inputs and end-to-end `TimeExpr`/`TimeRange` cases.

Signed-off-by: yifei.ma <yifeima98@gmail.com>

@oteffahi oteffahi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nitpick. Otherwise LGTM.

Comment thread commons/zenoh-util/src/time_range.rs Outdated
@oteffahi oteffahi added the bug Something isn't working label Jun 16, 2026
Co-authored-by: Oussama Teffahi <70609372+oteffahi@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants