Skip to content
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

Overhaul Utility::FormatDateTime() #10112

Merged
merged 6 commits into from
Aug 26, 2024
Merged

Overhaul Utility::FormatDateTime() #10112

merged 6 commits into from
Aug 26, 2024

Conversation

julianbrost
Copy link
Contributor

@julianbrost julianbrost commented Aug 8, 2024

What started as a small "there's something strange happening" (that was the Windows invalid format string thing from the following list), it turned out that there was a lot of room for improvement within this function, so this PR does the following:

  • Better input validation, the old code used a cast that was technically undefined behavior.
  • Remove the use of a thread-unsafe function on Windows.
  • Use the return value of strftime() to specify the length of the returned string, which prevents undefined behavior as the function makes no guarantees about the buffer contents when an error occurred.
  • Add a workaround for invalid format strings on Windows (the C library there calls an invalid input handler on an invalid format string which may terminate the process).
  • Add tests for the function in general and for the fixed edge-cases.
  • Add an overload to allow calling the function with tm* as well (trivial implementation-wise, it just meant splitting the function in the middle) and replace a similar ad-hoc implementation within the tests.

The individual commit messages contain more details on the individual points.

Note that I initially wanted to use std::put_time() which does not have a fixed size output buffer and should allow using the error handling of the ostream. However, there seem to be an unfortunate implementation of this on some Windows versions where passing an invalid format string results in std::bad_alloc and the process allocating more and more memory before throwing the exception. I've kept the use of strftime() for now as the process trying to use all memory is arguable worse than the limitations of strftime(). Also, I can't remember that there were any complaints about that 127 byte limit and it should also be reasonable for normal use.

ref/IP/54443

@cla-bot cla-bot bot added the cla/signed label Aug 8, 2024
@julianbrost julianbrost force-pushed the formatdatetime branch 2 times, most recently from 83085f2 to 5660595 Compare August 9, 2024 08:31
@julianbrost julianbrost added bug Something isn't working ref/IP labels Aug 9, 2024
@Al2Klimov
Copy link
Member

I guess just checking the return of strftime() != 0 would be totally fine.

strftime(timestamp, sizeof(timestamp), format, &tmthen);

@julianbrost
Copy link
Contributor Author

I guess just checking the return of strftime() != 0 would be totally fine.

And then do what? Note that a return value of 0 doesn't necessarily indicate an error, if format = "", that would be valid and successful. So that would only be nice if we'd want to return the empty string on error. Otherwise, you could try to add some errno checks, but that's what I wanted to avoid by just using the C++ mechanism.

And you could also omit the stream.exceptions(), then it would also return an empty string, so I see little reason to stick with the more error-prone C function if there's something a bit more robust in C++.

@julianbrost julianbrost force-pushed the formatdatetime branch 4 times, most recently from 185d393 to 27aabf9 Compare August 21, 2024 12:35
@julianbrost julianbrost changed the title Utility::FormatDateTime: use std::put_time Overhaul Utility::FormatDateTime() Aug 22, 2024
The previous implementation actually had undefined behavior when called with a
double that can't be represented as time_t. With boost::numeric_cast, there's a
convenient cast available that avoids this and throws an exceptions on
overflow.

It's undefined behavior ([0], where the implicit conversion rule comes into
play because the C-style cast uses static_cast [1] which in turn uses the
imlicit conversion as per rule 5 of [2]):

> A prvalue of floating-point type can be converted to a prvalue of any integer
> type. The fractional part is truncated, that is, the fractional part is
> discarded.
>
> * If the truncated value cannot fit into the destination type, the behavior
>   is undefined (even when the destination type is unsigned, modulo arithmetic
>   does not apply).

Note that on Linux amd64, the undefined behavior typically manifests itself in
the result being the minimal value of time_t which then results in localtime_r
failing with EOVERFLOW.

[0]: https://en.cppreference.com/w/cpp/language/implicit_conversion#Floating.E2.80.93integral_conversions
[1]: https://en.cppreference.com/w/cpp/language/explicit_cast
[2]: https://en.cppreference.com/w/cpp/language/static_cast
localtime() is not thread-safe as it returns a pointer to a shared tm struct.
Everywhere except on Windows, localtime_r() is used already which avoids the
problem by using a struct allocated by the caller for the output.

Windows actually has a similar function called localtime_s() which has the same
properties, just with a different name and order of arguments.
So far, the return value of strftime() was simply ignored and the output buffer
passed to the icinga::String constructor. However, there are error conditions
where strftime() returns 0 to signal an error, like if the buffer was too small
for the output. In that case, there's no guarantee on the buffer contents and
reading it can result in undefined behavior. Unfortunately, returning 0 can
also indicate success and strftime() doesn't set errno, so there's no reliable
way to distinguish both situations. Thus, the implementation now returns the
empty string in both cases.

I attempted to use std::put_time() at first as that allows for better error
handling, however, there were problems with the implementation on Windows (see
inline comment), so I put that plan on hold at left strftime() there for the
time being.
On Windows, the strftime() function family invokes an invalid parameter handler
when the format string is invalid (see the "Remarks" section in their
documentation). std::put_time() shows the same behavior as it uses
_wcsftime_l() internally. The default invalid parameter handler may terminate
the process, which can be a problem given that the format string can be
specified by the user from the Icinga DSL.

Thus, temporarily set a thread-local no-op handler to disable the default one
allowing the program to continue. This then simply results in the function
returning an error which then results in an exception as we ask the stream to
throw one.

See also:
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/strftime-wcsftime-strftime-l-wcsftime-l?view=msvc-170
https://learn.microsoft.com/en-us/cpp/c-runtime-library/parameter-validation?view=msvc-170
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/set-invalid-parameter-handler-set-thread-local-invalid-parameter-handler?view=msvc-170
This allows the function to be used both with a double timestamp or a pointer
to a tm struct. With this, a similar implementation inside the tests can simply
use our regular function.
@julianbrost julianbrost marked this pull request as ready for review August 23, 2024 12:12
@julianbrost julianbrost added this to the 2.15.0 milestone Aug 23, 2024
@julianbrost julianbrost merged commit 145bb61 into master Aug 26, 2024
26 checks passed
@julianbrost julianbrost deleted the formatdatetime branch August 26, 2024 12:31
@julianbrost julianbrost added the consider backporting Should be considered for inclusion in a bugfix release label Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cla/signed consider backporting Should be considered for inclusion in a bugfix release ref/IP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants