overly relaxed whitespace parsing leads to unexpected parsing success (was Local.datetime_from_str errantly ignoring format whitespace)

[jtmoon79]

Function chrono::Local.datetime_from_str ignores leading or trailing whitespace in the passed variable fmt.

Given code

extern crate chrono;
use chrono::{Local, TimeZone};

fn main() {
    let a = Local.datetime_from_str("2000-01-01 02:03:01", "%Y-%m-%d %H:%M:%S ");
    println!("{:?}", a);
    let a = Local.datetime_from_str("2000-01-01 02:03:02", " %Y-%m-%d %H:%M:%S ");
    println!("{:?}", a);
    let a = Local.datetime_from_str("2000-01-01 02:03:03\n", "%Y-%m-%d %H:%M:%S ");
    println!("{:?}", a);
}

results in unexpected successful parsing.

Ok(2000-01-01T02:03:01+00:00)
Ok(2000-01-01T02:03:02+00:00)
Ok(2000-01-01T02:03:03+00:00)

I expected

Err(ParseError(Invalid))
Err(ParseError(Invalid))
Err(ParseError(Invalid))

The passed variable fmt has a surrounding whitespace that is not present in the passed s, yet datetime_from_str returns Ok. These are errant matches.

---

Using chrono version 0.4.19.

Similar to Issue #548

[jtmoon79]

In case anyone needs a hack workaround. (full code with tests)

/// workaround for chrono Issue #660 https://github.com/chronotope/chrono/issues/660
/// match spaces at beginning and ending of inputs
/// TODO: handle all Unicode whitespace.
///       This fn is essentially counteracting an errant call to `std::string:trim`
///       within `Local.datetime_from_str`.
///       `trim` removes "Unicode Derived Core Property White_Space".
///       This implementation handles three whitespace chars. There are twenty-five whitespace
///       chars according to
///       https://en.wikipedia.org/wiki/Unicode_character_property#Whitespace .
pub fn datetime_from_str_workaround_Issue660(value: &str, pattern: &str) -> bool {
    let spaces = " ";
    let tabs = "\t";
    let lineends = "\n\r";
    
    // match whitespace forwards from beginning
    let mut v_sc: u32 = 0;  // `value` spaces count
    let mut v_tc: u32 = 0;  // `value` tabs count
    let mut v_ec: u32 = 0;  // `value` line ends count
    let mut v_brk: bool = false;
    for v_ in value.chars() {
        if spaces.contains(v_) {
            v_sc += 1;
        } else if tabs.contains(v_) {
            v_tc += 1;
        } else if lineends.contains(v_) {
            v_ec += 1;
        } else {
            v_brk = true;
            break;
        }
    }
    let mut p_sc: u32 = 0;  // `pattern` space count
    let mut p_tc: u32 = 0;  // `pattern` tab count
    let mut p_ec: u32 = 0;  // `pattern` line ends count
    let mut p_brk: bool = false;
    for p_ in pattern.chars() {
        if spaces.contains(p_) {
            p_sc += 1;
        } else if tabs.contains(p_) {
            p_tc += 1;
        } else if lineends.contains(p_) {
            p_ec += 1;
        } else {
            p_brk = true;
            break;
        }
    }
    if v_sc != p_sc || v_tc != p_tc || v_ec != p_ec {
        return false;
    }
    
    // match whitespace backwards from ending
    v_sc = 0;
    v_tc = 0;
    v_ec = 0;
    if v_brk {
        for v_ in value.chars().rev() {
            if spaces.contains(v_) {
                v_sc += 1;
            } else if tabs.contains(v_) {
                v_tc += 1;
            } else if lineends.contains(v_) {
                v_ec += 1;
            } else {
                break;
            }
        }
    }
    p_sc = 0;
    p_tc = 0;
    p_ec = 0;
    if p_brk {
        for p_ in pattern.chars().rev() {
            if spaces.contains(p_) {
                p_sc += 1;
            } else if tabs.contains(p_) {
                p_tc += 1;
            } else if lineends.contains(p_) {
                p_ec += 1;
            } else {
                break;
            }
        }
    }
    if v_sc != p_sc || v_tc != p_tc || v_ec != p_ec {
        return false;
    }
    
    return true;
}

pub fn main() {
    for (dts, pattern) in &[
        // should not match
        ("2000-01-01 02:03:01", "%Y-%m-%d %H:%M:%S "),
        ("2000-01-01 02:03:02", " %Y-%m-%d %H:%M:%S "),
        ("2000-01-01 02:03:03\n", "%Y-%m-%d %H:%M:%S "),
        // should match
        ("2000-01-01 02:03:04 ", "%Y-%m-%d %H:%M:%S "),
        (" 2000-01-01 02:03:05", " %Y-%m-%d %H:%M:%S"),
    ] {
        let dt = match Local.datetime_from_str(dts, pattern) {
            Ok(val) => {
                // HACK: workaround chrono Issue #660 by checking for matching begin, end of `dts`
                //       and `pattern`
                if !datetime_from_str_workaround_Issue660(dts, pattern) {
                    None
                } else {
                    Some(val)
                }
            }
            Err(err) => {None}
        };
        println!("{:?}", dt)
    }
   
}

console prints

None
None
None
Some(2000-01-01T02:03:04+00:00)
Some(2000-01-01T02:03:05+00:00)

[djc]

I think I agree that we should fix this. Would you be able to submit a PR to fix this, along with some test cases similar to the ones you already have here? That would be great!

[jtmoon79]

@djc I posted a proof-of-concept PR at #786

This does fix this Issue #660.

However, while working on this Issue, I noticed the tendency to trim arbitrary amounts of whitespace happens in many places:

$ git grep -C1 -E 'is_whitespace|trim_left|trim_right|trim_start|trim_end'
src/format/parse.rs-
src/format/parse.rs:    s = s.trim_left();
src/format/parse.rs-
--
src/format/parse.rs-
src/format/parse.rs:    s = s.trim_left();
src/format/parse.rs-    parsed.set_day(try_consume!(scan::number(s, 1, 2)))?;
--
src/format/parse.rs-    parsed.set_hour(try_consume!(scan::number(s, 2, 2)))?;
src/format/parse.rs:    s = scan::char(s.trim_left(), b':')?.trim_left(); // *S ":" *S
src/format/parse.rs-    parsed.set_minute(try_consume!(scan::number(s, 2, 2)))?;
src/format/parse.rs:    if let Ok(s_) = scan::char(s.trim_left(), b':') {
src/format/parse.rs-        // [ ":" *S 2DIGIT ]
--
src/format/parse.rs-                        eprintln!("c {:?}", c);
src/format/parse.rs:                        if ! c.is_whitespace() {
src/format/parse.rs-                            // `s` whitespace must match each `item`
--
src/format/parse.rs-            Item::OwnedSpace(_) => {
src/format/parse.rs:                s = s.trim_left();
src/format/parse.rs-            }
--
src/format/parse.rs-
src/format/parse.rs:                s = s.trim_left();
src/format/parse.rs-                let v = if signed {
--
src/format/parse.rs-                        let offset = try_consume!(scan::timezone_offset(
src/format/parse.rs:                            s.trim_left(),
src/format/parse.rs-                            scan::colon_or_space
--
src/format/parse.rs-                        let offset = try_consume!(scan::timezone_offset_zulu(
src/format/parse.rs:                            s.trim_left(),
src/format/parse.rs-                            scan::colon_or_space
--
src/format/parse.rs-                        let offset = try_consume!(scan::timezone_offset_permissive(
src/format/parse.rs:                            s.trim_left(),
src/format/parse.rs-                            scan::colon_or_space
--
src/format/scan.rs-    // if there are more than 9 digits, skip next digits.
src/format/scan.rs:    let s = s.trim_left_matches(|c: char| ('0'..='9').contains(&c));
src/format/scan.rs-
--
src/format/scan.rs-pub(super) fn space(s: &str) -> ParseResult<&str> {
src/format/scan.rs:    let s_ = s.trim_left();
src/format/scan.rs-    if s_.len() < s.len() {
--
src/format/scan.rs-pub(super) fn colon_or_space(s: &str) -> ParseResult<&str> {
src/format/scan.rs:    Ok(s.trim_left_matches(|c: char| c == ':' || c.is_whitespace()))
src/format/scan.rs-}
--
src/format/scan.rs-pub(super) fn timezone_name_skip(s: &str) -> ParseResult<(&str, ())> {
src/format/scan.rs:    Ok((s.trim_left_matches(|c: char| !c.is_whitespace()), ()))
src/format/scan.rs-}
--
src/format/scan.rs-
src/format/scan.rs:    let s = s.trim_start();
src/format/scan.rs-
--
src/format/strftime.rs-            // the next item is space
src/format/strftime.rs:            Some(c) if c.is_whitespace() => {
src/format/strftime.rs-                // `%` is not a whitespace, so `c != '%'` is redundant
--
src/format/strftime.rs-                    .remainder
src/format/strftime.rs:                    .find(|c: char| !c.is_whitespace())
src/format/strftime.rs-                    .unwrap_or(self.remainder.len());
--
src/format/strftime.rs-                    .remainder
src/format/strftime.rs:                    .find(|c: char| c.is_whitespace() || c == '%')
src/format/strftime.rs-                    .unwrap_or(self.remainder.len());

(I have only glanced at each statement listed).

It seems like the "right solution" is to change this chrono behavior "silently consume arbitrary whitespace" to "be exacting about whitespace characters".
To express differently, changing only datetime_from_str and underlying parse and parse_internal to be picky about whitespace (and not change other functions) seems like it might introduce more confusion than is worthwhile.

Do you have any thoughts on this?
Is modifying only parse_internal (as done in the PR) a good solution? Or should a larger wholistic solution be considered? Or should changes be abandoned because they would upset too many users of chrono?

My own preference is that whitespace and character literals should matter when doing "matching" type things. Silently accepting arbitrary noise is imprecise and unexpected. My concern is how much user code could break.

(again, I haven't taken the time to deeply understand all chrono whitespace handling in the git grep listing, only skimmed through some of it).

[djc]

Well, some uses of trimming are legit (that is, legal in terms of the grammar for the relevant format). But yes, I do share your worry that we might break too many users. Maybe this is something we should only change in the 0.5 release.

@esheppa what do you think?

[esheppa]

I think waiting for 0.5 is reasonable here. One way to eliminate these kind of issues is to have it parsed at compile time via something like #735

One thing that I'd like to do is improve the error messages from the parser, which would be ideal to combine with any increasing strictness, to ensure that if/when we do break user code, at least fixing it is easier. (For example the parser could allocate a String in the error path, highlighting the part or parts of the input that are causing problems, but there could also be other functions that don't allocate on error for performance sensitive use cases)

[djc]

(For example the parser could allocate a String in the error path, highlighting the part or parts of the input that are causing problems, but there could also be other functions that don't allocate on error for performance sensitive use cases)

Also for no_std users...