Skip to content

bpo-36020: Remove snprintf macro in pyerrors.h #20889

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
Jun 15, 2020
Merged

bpo-36020: Remove snprintf macro in pyerrors.h #20889

merged 2 commits into from
Jun 15, 2020

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jun 15, 2020

On Windows, #include "pyerrors.h" no longer defines "snprintf" and
"vsnprintf" macros.

PyOS_snprintf() and PyOS_vsnprintf() should be used to get portable
behavior.

Replace snprintf() calls with PyOS_snprintf() and replace vsnprintf()
calls with PyOS_vsnprintf().

https://bugs.python.org/issue36020

On Windows, #include "pyerrors.h" no longer defines "snprintf" and
"vsnprintf" macros.

PyOS_snprintf() and PyOS_vsnprintf() should be used to get portable
behavior.

Replace snprintf() calls with PyOS_snprintf() and replace vsnprintf()
calls with PyOS_vsnprintf().
@vstinner
Copy link
Member Author

@vstinner
Copy link
Member Author

PyOS_snprintf() and PyOS_vsnprintf() should be used to get portable behavior.

https://docs.python.org/dev/c-api/conversion.html#c.PyOS_snprintf

"PyOS_snprintf() and PyOS_vsnprintf() wrap the Standard C library functions snprintf() and vsnprintf(). Their purpose is to guarantee consistent behavior in corner cases, which the Standard C functions do not."

@skrah
Copy link
Contributor

skrah commented Jun 15, 2020

Which snprintf implementations are broken?

@skrah
Copy link
Contributor

skrah commented Jun 15, 2020

The original PyOS_snprintf is from 2001. Is the stdlib snprintf really still broken on platforms?

@vstinner
Copy link
Member Author

Which snprintf implementations are broken?

This PR is about removing "snprintf" and "vsnprintf" macros from Python.h.

I consider proposing a follow-up macro to deprecate PyOS_ functions and use directly snprintf() and vsnprintf() functions. But I didn't have time to dig into the history and try to understand the rationale behind these wrappers. So for now, it's safer to use PyOS_ wrappers.

Currently, PyOS_vsnprintf() has a fallback implementation with this:

    else if ((size_t)len >= size + _PyOS_vsnprintf_EXTRA_SPACE) {
        _Py_FatalErrorFunc(__func__, "Buffer overflow");
    }

Maybe this code is now dead since all platforms that we support are providing vsnprintf(). My plan is first to try to remove the fallback and see if it breaks any buildbot :-)

@vstinner
Copy link
Member Author

Ah, also, this PR is intended to land into 3.9, so I prefer to take the safest approach for this PR.

For the follow-up PR, we can break stuff :-)

@skrah
Copy link
Contributor

skrah commented Jun 15, 2020

So for now, it's safer to use PyOS_ wrappers.

I kind of disagree, because snprintf has been extremely heavily tested in _decimal and the other function has not.

@vstinner
Copy link
Member Author

I reverted changes in the _decimal module.

@skrah
Copy link
Contributor

skrah commented Jun 15, 2020

Thanks, I've definitely always compiled _decimal.c with -std=c99, so e.g. this passage in Python/mysnprintf.c makes me uneasy:

CAUTION: Unlike C99, str != NULL and size > 0 are required.

@vstinner vstinner merged commit e822e37 into python:master Jun 15, 2020
@miss-islington
Copy link
Contributor

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

@vstinner vstinner deleted the snprintf branch June 15, 2020 19:59
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 15, 2020
On Windows, GH-include "pyerrors.h" no longer defines "snprintf" and
"vsnprintf" macros.

PyOS_snprintf() and PyOS_vsnprintf() should be used to get portable
behavior.

Replace snprintf() calls with PyOS_snprintf() and replace vsnprintf()
calls with PyOS_vsnprintf().
(cherry picked from commit e822e37)

Co-authored-by: Victor Stinner <vstinner@python.org>
@bedevere-bot
Copy link

GH-20897 is a backport of this pull request to the 3.9 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Jun 15, 2020
miss-islington added a commit that referenced this pull request Jun 15, 2020
On Windows, GH-include "pyerrors.h" no longer defines "snprintf" and
"vsnprintf" macros.

PyOS_snprintf() and PyOS_vsnprintf() should be used to get portable
behavior.

Replace snprintf() calls with PyOS_snprintf() and replace vsnprintf()
calls with PyOS_vsnprintf().
(cherry picked from commit e822e37)

Co-authored-by: Victor Stinner <vstinner@python.org>
arun-mani-j pushed a commit to arun-mani-j/cpython that referenced this pull request Jul 21, 2020
On Windows, #include "pyerrors.h" no longer defines "snprintf" and
"vsnprintf" macros.

PyOS_snprintf() and PyOS_vsnprintf() should be used to get portable
behavior.

Replace snprintf() calls with PyOS_snprintf() and replace vsnprintf()
calls with PyOS_vsnprintf().
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.

5 participants