Skip to content

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

Merged
vstinner merged 2 commits into
python:masterfrom
vstinner:snprintf
Jun 15, 2020
Merged

bpo-36020: Remove snprintf macro in pyerrors.h#20889
vstinner merged 2 commits into
python:masterfrom
vstinner:snprintf

Conversation

@vstinner

@vstinner vstinner commented Jun 15, 2020

Copy link
Copy Markdown
Member

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
Copy Markdown
Member Author

@vstinner

Copy link
Copy Markdown
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

skrah commented Jun 15, 2020

Copy link
Copy Markdown
Contributor

Which snprintf implementations are broken?

@skrah

skrah commented Jun 15, 2020

Copy link
Copy Markdown
Contributor

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

@vstinner

Copy link
Copy Markdown
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
Copy Markdown
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

skrah commented Jun 15, 2020

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Member Author

I reverted changes in the _decimal module.

@skrah

skrah commented Jun 15, 2020

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown

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

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>
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