Skip to content

Conversation

JKRhb
Copy link
Contributor

@JKRhb JKRhb commented Sep 28, 2022

This PR is a small follow-up to #126, addressing two warnings raised by Visual Studio when compiling for Windows.

In order to address the warnings, this PR introduces a platform-specific print_timestamp function, using localtime_s instead of localtime (which led to the emission of warnings for being insecure). Furthermore, the definition of _CRT_RAND_S is moved to tinydtls.h as it initializes the global state of the rand_s function and defining it in dtls_prng_win.c was apparently too late in the compilation process.

@JKRhb JKRhb marked this pull request as ready for review September 28, 2022 14:13
static inline size_t
print_timestamp(char *s, size_t len, time_t t) {
struct tm tmp;
errno_t err = localtime_s(&tmp, &t);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not a that good practice, to use the OS instead of a specific HAVE_, if there is no good reason.

So, what is the warning here, you like to fix?

(Just, if you prefer, split the _CRT_RAND_S stuff in a second PR, that may be faster to merge.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your feedback on this, @boaks. I will shortly move the _CRT_RAND_S change to a separate PR.

Regarding the localtime change: IIRC, the warning it addresses is the following: C4996 'localtime': This function or variable may be unsafe. Consider using localtime_s instead

Do you have a suggestion how this could be solved in a better way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The _CRT_RAND_S change is now included separately in #178.

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.

2 participants