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

Fix Backwards fopen_s Check #98

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

Calandracas606
Copy link
Contributor

fopen_s, (and the defined macro), return 0 on success, but the if statement is expecting a non-zero value on success.

This causes cl_util_write_binaries to fail, even though it is successfully opening a file.

Can be tested very easily by building and running the "binaries" sample program.

fopen_s, (and the defined macro), return 0 on success.
The if statement is broken.
This can be easily tested by running the "binaries" sample
@Calandracas606
Copy link
Contributor Author

Would it be reasonable to simply replace the fopen_s() calls with equivilant fopen()?

fopen_s is not widely supported, and can cause confusion, especially when it is redefined as a macro

Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

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

Thank you for the fix!

Longer term I think it would be better (less cognitive burden) to change this to use the IF_ERR macro similar to the other calls to fopen_s, which would keep the error condition consistent. This can happen in a subsequent PR, though.

@bashbaug
Copy link
Contributor

Would it be reasonable to simply replace the fopen_s() calls with equivilant fopen()?

We discussed this in our weekly teleconference and I don't think there's a right answer here: I agree that the "secure" fopen_s widely supported right now, but we also want the SDK to build warning-free on all of our configurations without needing things like _CRT_SECURE_NO_WARNINGS, so we're going to stick with fopen_s for now. We will re-evaluate if we keep running into issues like these in the future - thanks!

Copy link
Collaborator

@MathiasMagnus MathiasMagnus left a comment

Choose a reason for hiding this comment

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

Ship it.

@bashbaug bashbaug merged commit 2db34c2 into KhronosGroup:main Feb 2, 2024
20 checks passed
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