Roguelazer/rust-syslog-rfc5424

Incorrect timezone parsing?

Closed this issue · 2 comments

RFC5424:

The TIMESTAMP field is a formalized timestamp derived from [RFC3339].

RFC3339:

Numeric offsets are calculated as "local time minus UTC". So the
equivalent time in UTC can be determined by subtracting the offset
from the local time. For example, 18:50:00-04:00 is the same time as
22:50:00Z. (This example shows negative offsets handled by adding
the absolute value of the offset.)

let utc_offset_mins = match rest.chars().next() {
None => 0,
Some('Z') => {
rest = &rest[1..];
0
}
Some(c) => {
let (sign, irest) = match c {
'+' => (1, &rest[1..]),
'-' => (-1, &rest[1..]),
_ => {
return Err(ParseErr::InvalidUTCOffset);
}
};
let hours = i32::from_str(&irest[0..2]).map_err(ParseErr::IntConversionErr)?;
let minutes = i32::from_str(&irest[3..5]).map_err(ParseErr::IntConversionErr)?;
rest = &irest[5..];
minutes + hours * 60 * sign
}
};
tm = tm + time::Duration::minutes(i64::from(utc_offset_mins));

This crate appears to add the timezone instead of subtracting it which contradicts the RFC. For example converting a local timestamp to UTC in postgresql gives a different result than this parser. I'm also not sure about the part where the sign is only applied to the hours part, not the minutes part.

In any case I really don't think this crate should implement its own timestamp parser - or try to parse timestamps at all, for that matter. Not every application that needs syslog parsing necessarily needs to parse the timestamps (keeping them as strings is just fine for my case) and the parser in here is both handwritten and based on the deprecated time crate. If I do need to parse a timestamp string I can always just throw it into the chrono crate with a single parse() call.

You're right, this is backwards. I will fix that momentarily.

I don't think we're going to drop timestamp parsing, both because it would be backwards-incompatible and because my confidence that chrono is going to last longer than time is pretty minimal, but my confidence in the portability of a time_t is pretty high.

This is fixed in v0.6.1, which is now available on crates.io