Skip to content

fix: change the format of the ascTime() function #3398

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: v3/master
Choose a base branch
from

Conversation

airween
Copy link
Member

@airween airween commented Jun 7, 2025

what

This PR changes the format of utils::string::ascTime().

The function is used only in one place, in transaction.cc, it produces the field time_stamp if the audit log format is JSON, for eg.:

{"transaction":{"client_ip":"200.249.12.31","time_stamp":"Sat Jun 07 11:46:23 2025"}}

why

The old (actual) format of that function is %c, which is the expected format, but as the documentation writes:

writes standard date and time string, e.g. Sun Oct 17 04:41:13 2010 (locale dependent)

it's locale dependent. This means if the locale uses only 1 digit for the day, then the test is failed, see this comment. The log in that case will look like this:

{"transaction":{"client_ip":"200.249.12.31","time_stamp":"Sat Jun  7 11:46:23 2025"}}

Please note that there are two spaces and 1 digit.

This modification changes the format and if the day is below 10, then adds a leading zero, without extra space.

references

C++ reference of strftime.

other notes

Please review this PR carefully as it may break audit log file JSON parsers if the server localization uses a single-digit date representation.

@airween airween requested a review from theseion June 7, 2025 10:09
@airween
Copy link
Member Author

airween commented Jun 7, 2025

@JakubOnderka could you take a review too?

@theseion
Copy link
Collaborator

theseion commented Jun 7, 2025

I was confused as to why this fix is necessary, so I looked into the LLVM implementation. It turns out that the use %a %b %e %T %Y (%e instead of %c) but just use a fixed size for the output string:
https://github.com/llvm/llvm-project/blob/995d74f8663edb2e20f2d2672556582a6f4cc3f1/libc/src/time/strftime_core/composite_converter.h#L180. That explains the additional space.

@theseion theseion self-requested a review June 7, 2025 12:20
@airween
Copy link
Member Author

airween commented Jun 7, 2025

so I looked into the LLVM implementation.

Thanks for reviewing that.

Moreover, your suggestion to %T makes sense, this escaped my attention, thanks too.

Unfortunately, after my initial commit the error still persists, even on Linux (with gcc).

Co-authored-by: Max Leske <250711+theseion@users.noreply.github.com>
Copy link

sonarqubecloud bot commented Jun 7, 2025

@airween
Copy link
Member Author

airween commented Jun 8, 2025

Hmm... all test have passed. It's very weird.

Note, that the original problem was (in my opinion) that previously the test expected this format: \S{3} \S{3} \d{2}, but yesterday the output was Sat Jun 7, which didn't match.

@JakubOnderka
Copy link
Contributor

@airween I am not sure if changing that is the best idea as even the new format will still be problematic. There is no good solution for that, but maybe the best will be include new field (maybe something like like timestamp_iso) that will include timestamp in ISO 8601 format (2025-06-09T12:30:00Z), that is much easier to parse and also contains timezone (that is crucial when comparing logs from different systems).

@airween
Copy link
Member Author

airween commented Jun 9, 2025

@JakubOnderka,

thanks for reviewed this.

@airween I am not sure if changing that is the best idea as even the new format will still be problematic. There is no good solution for that, but maybe the best will be include new field (maybe something like like timestamp_iso) that will include timestamp in ISO 8601 format (2025-06-09T12:30:00Z), that is much easier to parse and also contains timezone (that is crucial when comparing logs from different systems).

ah, my bad... I thought this issue is a serious one, and I didn't understand why was it failed. I assumed this tests is an old one, but meanwhile I realized that this is a new test - you added that in one of your recent commit.

So we can keep this format as is, but please change that test pattern:

      \"time_stamp\":\"\\S{3} \\S{3} \\d{2} \\d{2}:\\d{2}:\\d{2} \\d{4}\"",

The first \\d{2} is not good there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants