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

windows: address Visual Studio warnings #169

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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