fix: parse an epoch timestamp of exactly zero - #1739
Open
VXNCXNX wants to merge 1 commit into
Open
Conversation
ptime_s, ptime_i, ptime_6, ptime_9 and ptime_q reported success based on the parsed value being positive, so a timestamp of 0 read as a parse failure. Report success when digits were actually consumed instead. Fixes tstack#1586.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1586.
The cause
The epoch parsers all end by reporting success based on the value rather than on whether they parsed anything:
So a timestamp of exactly
0— the unix epoch itself — is indistinguishable from "no digits here", and lnav rejects the line. Your "Huh, that's an odd one... some sanity check somewhere" was on the money; the sanity check is the return statement itself.The fix
Report success when digits were actually consumed:
return (off_inout > off_start);off_startwas already declared inptime_sandptime_qfor their overflow guards;ptime_i,ptime_6andptime_9needed it added. The existingepoch >= MAX_TIME_Trange guards are untouched, and a field with no digits still fails because the loop consumes nothing.%qis included. The issue is about%s, butptime_q(hex epoch) has the identicalreturn (epoch > 0)and therefore the identical bug — a hex epoch of0fails to parse for the same reason. Fixing four of five would have left the same defect in the codebase, so I did all five. Say the word if you'd rather it were scoped down.Testing, and what I could not do
Being straight about this: I could not run
make check.autoconf,automake,cmakeandpkg-configare all absent from my environment — onlyg++is available.What I did instead was compile the real
src/ptimec.hhandsrc/ptimec_rt.ccagainst the repo's ownthird-party/dateheaders, with small stubs forconfig.h,lnav_log.hhand the three out-of-line time helpers, and exercise the five functions directly:5 failures before, 0 after, with the non-digit and normal-value cases unchanged in both directions.
Test cases are added to
test/test_date_time_scanner.ccin the existing style: one asserting all five conversions parse0to epoch zero, one asserting a digit-less field is still a failure. (xis not a hex digit, so it's a valid negative case for%qtoo — I checked.)A NEWS.md entry is added under
## lnav v0.14.1.The full doctest suite and any end-to-end log-file test remain unrun on my side.
Callers
ptime_fmtinsrc/ptimec_rt.ccis the only dispatcher and just propagates the boolean; its only in-tree consumer issrc/log_format.cc:3993.date_time_scanner.cchas no zero-value special case. Nothing depended on 0-means-failure.