ianmcook commented on code in PR #50146:
URL: https://github.com/apache/arrow/pull/50146#discussion_r4258635633
##########
cpp/src/arrow/util/value_parsing.h:
##########
@@ -826,10 +826,24 @@ static inline bool ParseTimestampStrptime(const char*
buf, size_t length,
if (!allow_trailing_chars && static_cast<size_t>(ret - clean_copy.c_str())
!= length) {
return false;
}
+ // Reject leap seconds, which Arrow cannot represent, instead of adding them
+ // to the time of day (which wraps them into the next minute); consistent
with
+ // the ISO-8601 parser. See GH-50146 for how strptime implementations and
+ // standards differ on these values.
+ if (result.tm_sec >= 60) {
+ return false;
+ }
+ // Reject impossible dates such as February 30, which strptime accepts (the
+ // day of month is only checked to be in 1..31), instead of rolling them over
+ // into the next month; consistent with the ISO-8601 parser.
+ const arrow_vendored::date::year_month_day ymd =
+ arrow_vendored::date::year(result.tm_year + 1900) / (result.tm_mon + 1) /
Review Comment:
Separately from the zero-day issue below: `result.tm_year + 1900` can
overflow `int`, and even when it doesn't, `date::year` only holds 16 bits, so
`ymd.ok()` can't catch an out-of-range year. Both are reachable from data with
a `%s` (epoch seconds) format. On macOS:
- `67768036191676799` triggers UBSan: `signed integer overflow: 2147483647 +
1900 cannot be represented in type 'int'`.
- `1000000000000000` (around year 31,690,708) wraps to year −28716, so a
`date32` column gets `-28716-07-05` and a `timestamp[s]` column gets
`-28716-07-05 01:46:40`.
This already happened for timestamp columns before this PR, but the PR makes
it reachable for date columns and rewrites this line, so it seems worth fixing
here. Could we compute the year in 64 bits and reject it if `date::year` can't
hold it?
```cpp
// tm_year + 1900 can overflow int (e.g. with %s), and date::year only
// holds 16 bits
const int64_t year = static_cast<int64_t>(result.tm_year) + 1900;
if (year < static_cast<int>(arrow_vendored::date::year::min()) ||
year > static_cast<int>(arrow_vendored::date::year::max())) {
return false;
}
```
and then build `ymd` from
`arrow_vendored::date::year(static_cast<int>(year))`. I checked that this
rejects both values above with no UBSan report, and leaves `1584282600` with
`%s`, years 0000 and 9999, and `2000-02` with `%Y-%m` unchanged. A regression
test needs `%s`, which the vendored strptime used on Windows doesn't support,
so it would have to be skipped there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
