Skip to content

Replace usage of std::ctime, which is not safe in multithread contexts #3228

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

Merged
merged 2 commits into from
Aug 14, 2024

Conversation

eduar-hte
Copy link
Contributor

what

Replace usage of std::ctime in the string util ascTime, which is not safe in multithreaded contexts. ascTime is used by Transaction::toJSON, which is used to log transactions.

why

From IBM's ctime documentation:

The asctime() and ctime() functions, and other time functions can use a common, statically allocated buffer to hold the return string. Each call to one of these functions might destroy the result of the previous call. The asctime_r(), ctime_r(), gmtime_r(), and localtime_r() functions do not use a common, statically allocated buffer to hold the return string. These functions can be used in place of asctime(), ctime(), gmtime(), and localtime() if reentrancy is desired.

This was reported by sonarcloud while working on PR #3222:

Remove use of this obsolete "ctime" function. Replace it by a call to "strftime".
Obsolete POSIX functions should not be used cpp:S1911

changes

  • Replace usage of std::ctime with a call to strftime, where the output buffer is provided as an argument and is thread-safe.
  • Reviewed and adjusted buffer size for others uses of strftime to leverage std::size to reserve the space needed by the format string.
  • Simplified implementation of TimeMon::evaluate which was unnecessarily generating a string representation of the month to then parse it as an integer value, when the value is already available in the struct tm timeinfo with the call to localtime_r.

notes

Checked that the library doesn't currently include any call to localtime (which is not thread-safe either) and uses localtime_r instead.

…ed contexts

- std::ctime returns a pointer to a string that "may be shared between
  std::asctime and std::ctime, and may be overwritten on each invocation
  of any of those functions.".
  - https://en.cppreference.com/w/cpp/chrono/c/ctime
- Replaced with call to strftime to generate the same string
  representation (using the format string: %c)
- Leveraged localtime_r (which is thread-safe) to convert time_t to
  struct tm, as required by strftime.
@eduar-hte eduar-hte force-pushed the asctime-multithread branch from e946f9c to 9cbd699 Compare August 13, 2024 20:02
@eduar-hte
Copy link
Contributor Author

eduar-hte commented Aug 13, 2024

The sonarcloud reported issues recommend replacing the use of strftime with C++20's std::chrono library and std::format. 🙄

Replace this use of "strftime" with equivalent code using "std::chrono".
"std::chrono" components should be used to operate on time cpp:S6229
(...)
The chrono library, introduced in C++20, provides support for calendars, time zones, and i/o formatting and parsing operations on time-related objects.

- Leverage std::size to determine buffer size at compile time.
- Simplified 'TimeMon::evaluate' implementation as it was using strftime
  to get the month, convert the string to int, and then decrement it by
  one to make it zero based. This same value is already available in
  the 'struct tm' previously generated with the call to localtime_r (and
  where the month is already zero-based)
@eduar-hte eduar-hte force-pushed the asctime-multithread branch from 4e9d63e to 23a341e Compare August 13, 2024 20:36
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
C Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@marcstern marcstern added the 3.x Related to ModSecurity version 3.x label Aug 14, 2024
Copy link
Member

@airween airween left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks!

@airween airween merged commit b4f5232 into owasp-modsecurity:v3/master Aug 14, 2024
48 of 49 checks passed
airween added a commit to airween/ModSecurity that referenced this pull request Aug 14, 2024
@airween
Copy link
Member

airween commented Aug 14, 2024

The sonarcloud reported issues recommend replacing the use of strftime with C++20's std::chrono library and std::format. 🙄

I saw.

I updated SonarCloud settings, now it checks the code with C++17 standards. We will see the result...

@eduar-hte
Copy link
Contributor Author

I updated SonarCloud settings, now it checks the code with C++17 standards. We will see the result...

That's great! I think that should reduce noise (that is, we won't need to dismiss issues because they don't apply, at least yet) Thx!

@eduar-hte eduar-hte deleted the asctime-multithread branch August 14, 2024 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Related to ModSecurity version 3.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants