gh-101703: use PyOS_snprintf instead of sprintf

[sobolevn]

Several points of interest:

0. sprintf is indeed insecure generally: https://rules.sonarsource.com/c/RSPEC-6069 But, for our cases - I was not able to find a single insecure place, so I don't mark this PR as :security:
1. snprintf is also listed as not secure here: https://clang.llvm.org/docs/analyzer/checkers.html#security-insecureapi-deprecatedorunsafebufferhandling-c
2. snprintf is reported to be slightly slower than sprintf

• Issue: Let's get rid of sprintf() #101703

[sobolevn]

Ok, the first part with constant sized elements is done. Now, more complex ones.

[sobolevn]

Looks like we have PyOS_snprintf instead of just snprintf 🤔

[sobolevn]

Almost done!

There are a couple left, but I don't quite understand the code and I will highly appreciate help / advice on these cases:

• cpython/Modules/_ctypes/_ctypes.c

  Lines 2642 to 2651 in 5839575

  	cp += sprintf(cp, "%x", Py_SAFE_DOWNCAST(index, Py_ssize_t, int));
  	while (target->b_base) {
  	bytes_left = sizeof(string) - (cp - string) - 1;
  	/* Hex format needs 2 characters per byte */
  	if (bytes_left < sizeof(Py_ssize_t) * 2) {
  	PyErr_SetString(PyExc_ValueError,
  	"ctypes object structure too deep");
  	return NULL;
  	}
  	cp += sprintf(cp, ":%x", Py_SAFE_DOWNCAST(target->b_index, Py_ssize_t, int));

• cpython/Objects/unicodeobject.c

  Line 736 in 5839575

  size = sprintf(str, "&#%d;", PyUnicode_READ(kind, data, i));

• cpython/Parser/string_parser.c

  Line 121 in 5839575

  sprintf(p, "\\U%08x", chr);

• cpython/Python/pystrtod.c

  Line 1242 in 5839575

  exp_len = sprintf(p, "%+.02d", exp);

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @sobolevn for commit 9bc0864 🤖

If you want to schedule another build, you need to add the :hammer: test-with-buildbots label again.

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @sobolevn for commit c7dd348 🤖

If you want to schedule another build, you need to add the :hammer: test-with-buildbots label again.

[sobolevn]

@Yhg1s @gvanrossum it is ready to be reviewed :)

[sobolevn]

I've made the requested changes, thanks for the suggestions 👍

[sobolevn]

@Yhg1s can you please review again? :)

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @sobolevn for commit 8a95f08 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

[sobolevn]

I've merged it with the latest main, new round of buildbot tests are required to make sure that all still works correctly.

Does anyone want to take a look once again, please? 😉

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @sobolevn for commit 3e8e61f 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @sobolevn for commit d82e315 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

[AlexWaygood]

This now has merge conflicts

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[vstinner]

sprintf is indeed insecure generally: https://rules.sonarsource.com/c/RSPEC-6069 But, for our cases - I was not able to find a single insecure place, so I don't mark this PR as :security:

In the past, I saw people introducing bugs and security vulnerabilities by attempting to make linters happy about getting rid of "dangerous functions".

Python provides advances functions to format strings in a safe way: PyBytes_FromFormat() and PyUnicode_FromFormat(). Moreover, there is also _PyBytesWriter and _PyUnicodeWriter internal API which can be used to "create" a new string in a safe way (without having to handle memory directly).

If a string is longer than expected, PyOS_snprintf() truncates the string, but your change ignores PyOS_snprintf() result and so the string is ignored silently. I am not convinced that these changes are worth it:

• It doesn't prevent to truncate strings.
• It's designed to prevent buffer overflow, but it doesn't show examples of possible buffer overfow.
• From my point of view, I don't see how it makes the code easier to maintain or easier to read.

For example, the PR changes FindAddress() to use PyOS_snprintf(), but it keeps alloca() which is IMO way more problematic. Using PyBytes_FromFormat() would be safer here: the function takes care of memory management and allocates memory on the heap, there is no risk of stack overflow.

If you want me to review this PR, I would prefer smaller PRs with a more precise scope and goal.

[vstinner]

In the past, I saw people introducing bugs and security vulnerabilities by attempting to make linters happy about getting rid of "dangerous functions".

The worst example that I recall:

• https://bugs.php.net/bug.php?id=55439
• Change introducing the security issue: php/php-src@7eb5bbb : "Fix more signed 1-bit bitfield, and let's use strlcpy/strlcat instead for these static string copies". Replace "unsafe" strncpy()/strncat() with "safe" strlcpy()/strncat().
• Fix : php/php-src@4f98090 (as Subversion commit)

Commit message of the fix:

commit 4f980905a0bff94807ea07cb897c0e4cd4e6b83f
Author: Stanislav Malyshev <stas@php.net>
Date:   Fri Aug 19 22:49:18 2011 +0000

    Unbreak crypt() (fix bug #55439)
    # If you want to remove static analyser messages, be my guest,
    # but please run unit tests after

Well, at that time, PHP had no automated test suite, and running tests showed the regression. But well, it's surprising how using "safer" function can introduce a major security vulnerability.

The bug allowed to bypass any kind of login page (implemented with crypt()) with any password...

[sobolevn]

I wanted to close this PR anyway, I think that we should do it on a per-usage approach. Easier to review, easier to merge.