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

Regression: snprintf emulation function calls snprintf, causing recursion problem #67

Closed
michaelrsweet opened this issue Dec 17, 2020 · 4 comments
Assignees
Labels
bug Something isn't working platform issue Issue is specific to an OS or desktop priority-high
Milestone

Comments

@michaelrsweet
Copy link
Member

I was a little too eager to replace the sprintf calls with sprintf in cups/snprintf.c, need to make them sprintf again and add a comment so we don't do this again.

Also investigate options for not using snprintf emulation on Windows... :/

@michaelrsweet michaelrsweet self-assigned this Dec 17, 2020
@michaelrsweet michaelrsweet added bug Something isn't working platform issue Issue is specific to an OS or desktop priority-high labels Dec 17, 2020
@michaelrsweet michaelrsweet added this to the v2.3.3op2 milestone Dec 17, 2020
@sicklittlemonkey
Copy link

I see this in config.h, but no notes about what the breaking behaviour is:
/* Windows snprintf/vsnprintf are non-conforming */

What (if any) versions of Windows or Visual Studio are targeted now? Today's MS is not my father's son's MS. ; - )

I noticed this in the docs for snprintf:

"Beginning with the UCRT in Visual Studio 2015 and Windows 10, snprintf is no longer identical to _snprintf. The snprintf function behavior is now C99 standard compliant."

A small victory after so many years, I know, but things are changing:

All required features of C11 and C17 are now supported ... Note that the optional features are not supported, so we do not claim C99 compliance. Learn more in our C11/C17 blogpost.

For many years Visual Studio has only supported C to the extent of it being required for C++. Things are about to change now that a conformant token-based preprocessor has been added to the compiler. With the advent of two new compiler switches, /std:c11 and /std:c17, we are officially supporting the latest ISO C language standards.

@michaelrsweet
Copy link
Member Author

@sicklittlemonkey I did some digging but wasn't able to find the original reason this was added. I suspect there was, at some point, some difference in behavior between the MSVC runtime and POSIX/C99, but I didn't bother to include that information in the comment or any of the various commit messages.

In my HTMLDOC project, I just use _snprintf, so maybe that is the solution here vs. using the emulation functions... 🤷‍♂️

@michaelrsweet
Copy link
Member Author

Reading through the documentation, I think the issue was the return value - the old MS snprintf didn't support passing a NULL buffer or returning the total formatted length (vs. the length of the string), which is used in the cupsd logging functions to format an arbitrarily long line. But since cupsd isn't something that can be compiled with Visual Studio, that doesn't seem particularly important... Also, it looks like the new, more standard snprintf implementation doesn't have this problem anyways so...

@michaelrsweet
Copy link
Member Author

[master af4fae4f8] Fix snprintf emulation function (Issue #67)
[master 5be9389db] Update config header files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working platform issue Issue is specific to an OS or desktop priority-high
Projects
None yet
Development

No branches or pull requests

2 participants