gh-112301: Add -Wformat=2 compiler option to NODIST #122474

nohlson · 2024-07-30T17:32:10Z

This adds -Wformat and -Wformat=2 to the CFLAGS_NODIST set of compiler flags. This is a warning flag that relates for format strings and for more information you can take a look at the OpenSSF guidance on this flag.

This does generate a few warnings in build_ubuntu and the new warning checker catches them catches them:

| {'kind': 'warning', 'locations': [{'finish': {'byte-column': 27, 'display-column': 27, 'line': 2857, 'file': '../cpython-ro-srcdir/Objects/unicodeobject.c', 'column': 27}, 'caret': {'byte-column': 21, 'display-column': 21, 'line': 2857, 'file': '../cpython-ro-srcdir/Objects/unicodeobject.c', 'column': 21}}], 'column-origin': 1, 'option': '-Wformat-nonliteral', 'children': [], 'option_url': 'https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wformat-nonliteral', 'message': 'format not a string literal, argument types not checked'}
| {'kind': 'warning', 'locations': [{'finish': {'byte-column': 27, 'display-column': 27, 'line': 2858, 'file': '../cpython-ro-srcdir/Objects/unicodeobject.c', 'column': 27}, 'caret': {'byte-column': 21, 'display-column': 21, 'line': 2858, 'file': '../cpython-ro-srcdir/Objects/unicodeobject.c', 'column': 21}}], 'column-origin': 1, 'option': '-Wformat-nonliteral', 'children': [], 'option_url': 'https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wformat-nonliteral', 'message': 'format not a string literal, argument types not checked'}
| {'kind': 'warning', 'locations': [{'finish': {'byte-column': 27, 'display-column': 27, 'line': 2862, 'file': '../cpython-ro-srcdir/Objects/unicodeobject.c', 'column': 27}, 'caret': {'byte-column': 21, 'display-column': 21, 'line': 2862, 'file': '../cpython-ro-srcdir/Objects/unicodeobject.c', 'column': 21}}], 'column-origin': 1, 'option': '-Wformat-nonliteral', 'children': [], 'option_url': 'https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wformat-nonliteral', 'message': 'format not a string literal, argument types not checked'}
| {'kind': 'warning', 'locations': [{'finish': {'byte-column': 27, 'display-column': 27, 'line': 2863, 'file': '../cpython-ro-srcdir/Objects/unicodeobject.c', 'column': 27}, 'caret': {'byte-column': 21, 'display-column': 21, 'line': 2863, 'file': '../cpython-ro-srcdir/Objects/unicodeobject.c', 'column': 21}}], 'column-origin': 1, 'option': '-Wformat-nonliteral', 'children': [], 'option_url': 'https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wformat-nonliteral', 'message': 'format not a string literal, argument types not checked'}
| {'kind': 'warning', 'locations': [{'finish': {'byte-column': 27, 'display-column': 27, 'line': 2867, 'file': '../cpython-ro-srcdir/Objects/unicodeobject.c', 'column': 27}, 'caret': {'byte-column': 21, 'display-column': 21, 'line': 2867, 'file': '../cpython-ro-srcdir/Objects/unicodeobject.c', 'column': 21}}], 'column-origin': 1, 'option': '-Wformat-nonliteral', 'children': [], 'option_url': 'https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wformat-nonliteral', 'message': 'format not a string literal, argument types not checked'}
| {'kind': 'warning', 'locations': [{'finish': {'byte-column': 27, 'display-column': 27, 'line': 2868, 'file': '../cpython-ro-srcdir/Objects/unicodeobject.c', 'column': 27}, 'caret': {'byte-column': 21, 'display-column': 21, 'line': 2868, 'file': '../cpython-ro-srcdir/Objects/unicodeobject.c', 'column': 21}}], 'column-origin': 1, 'option': '-Wformat-nonliteral', 'children': [], 'option_url': 'https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wformat-nonliteral', 'message': 'format not a string literal, argument types not checked'}
| {'kind': 'warning', 'locations': [{'finish': {'byte-column': 19, 'display-column': 19, 'line': 2871, 'file': '../cpython-ro-srcdir/Objects/unicodeobject.c', 'column': 19}, 'caret': {'byte-column': 17, 'display-column': 17, 'line': 2871, 'file': '../cpython-ro-srcdir/Objects/unicodeobject.c', 'column': 17}}], 'column-origin': 1, 'option': '-Wformat-nonliteral', 'children': [], 'option_url': 'https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wformat-nonliteral', 'message': 'format not a string literal, argument types not checked'}
| {'kind': 'warning', 'locations': [{'finish': {'byte-column': 27, 'display-column': 27, 'line': 2875, 'file': '../cpython-ro-srcdir/Objects/unicodeobject.c', 'column': 27}, 'caret': {'byte-column': 21, 'display-column': 21, 'line': 2875, 'file': '../cpython-ro-srcdir/Objects/unicodeobject.c', 'column': 21}}], 'column-origin': 1, 'option': '-Wformat-nonliteral', 'children': [], 'option_url': 'https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wformat-nonliteral', 'message': 'format not a string literal, argument types not checked'}
| {'kind': 'warning', 'locations': [{'finish': {'byte-column': 27, 'display-column': 27, 'line': 2876, 'file': '../cpython-ro-srcdir/Objects/unicodeobject.c', 'column': 27}, 'caret': {'byte-column': 21, 'display-column': 21, 'line': 2876, 'file': '../cpython-ro-srcdir/Objects/unicodeobject.c', 'column': 21}}], 'column-origin': 1, 'option': '-Wformat-nonliteral', 'children': [], 'option_url': 'https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wformat-nonliteral', 'message': 'format not a string literal, argument types not checked'}
| {'kind': 'warning', 'locations': [{'finish': {'byte-column': 27, 'display-column': 27, 'line': 2880, 'file': '../cpython-ro-srcdir/Objects/unicodeobject.c', 'column': 27}, 'caret': {'byte-column': 21, 'display-column': 21, 'line': 2880, 'file': '../cpython-ro-srcdir/Objects/unicodeobject.c', 'column': 21}}], 'column-origin': 1, 'option': '-Wformat-nonliteral', 'children': [], 'option_url': 'https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wformat-nonliteral', 'message': 'format not a string literal, argument types not checked'}
| {'kind': 'warning', 'locations': [{'finish': {'byte-column': 27, 'display-column': 27, 'line': 2881, 'file': '../cpython-ro-srcdir/Objects/unicodeobject.c', 'column': 27}, 'caret': {'byte-column': 21, 'display-column': 21, 'line': 2881, 'file': '../cpython-ro-srcdir/Objects/unicodeobject.c', 'column': 21}}], 'column-origin': 1, 'option': '-Wformat-nonliteral', 'children': [], 'option_url': 'https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wformat-nonliteral', 'message': 'format not a string literal, argument types not checked'}
| {'kind': 'warning', 'locations': [{'finish': {'byte-column': 28, 'display-column': 28, 'line': 23, 'file': '../cpython-ro-srcdir/Python/getversion.c', 'column': 28}, 'caret': {'byte-column': 19, 'display-column': 19, 'line': 23, 'file': '../cpython-ro-srcdir/Python/getversion.c', 'column': 19}}], 'column-origin': 1, 'option': '-Wformat-nonliteral', 'children': [], 'option_url': 'https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wformat-nonliteral', 'message': 'format not a string literal, argument types not checked'}

unicodeobject.c warnings look like they could be ignored since format strings for sprintf operations are pulled from a const array of const format strings, and the variable that indexes these arrays is set from constants. However if we add this file to .warningignore_ubuntu then if either of those things change maybe vulnerabilities could be introduced.

As for getversion.c it also could be ignored. I don't think there is too much of a concern in putting this in the warning ignore file.

This change should require pre-merge build bots @corona10

Attn: @hugovk

EDIT: Removed content from the issue that pertained to already merged tooling options

Issue: Consider applying flags for warnings about potential security issues #112301

bedevere-bot · 2024-07-31T01:43:14Z

🤖 New build scheduled with the buildbot fleet by @corona10 for commit 178bcc6 🤖

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

configure.ac

encukou · 2024-08-07T07:38:35Z

Python/getversion.c

@@ -19,8 +19,17 @@ void _Py_InitVersion(void)
 #else
    const char *buildinfo_format = "%.80s (%.80s) %.80s";
 #endif
+    // The format string is defined above and is observably safe.


Could it be switched to a #defined literal? That way it could look safe to the compiler, too.

I just pushed a change. I figure instead of going through the trouble defining a format string in the preprocessor #if/#else blocks and adding all of the diagnostic pragmas we can just put the PyOS_snprintf() with the relevant format string literals in the respective #if/#else blocks. We eliminate the root cause of the warning instead of ignore it.

encukou · 2024-08-07T11:30:14Z

Objects/unicodeobject.c

+        // is only assigned known constant values. Ignore warnings related
+        // to the format string not being a string literal.
+        #if defined(__GNUC__) || defined(__clang__)
+        #pragma GCC diagnostic push


Consider using _Py_COMP_DIAG_PUSH/_Py_COMP_DIAG_POP, and adding a macro like _Py_COMP_DIAG_IGNORE_DEPR_DECLS, to make this easier to port to other compilers.

@encukou created macro for ignoring format nonliterals and applied it to this block

encukou · 2024-09-18T08:37:26Z

I wanted to see if it's possible to avoid the warning altogether -- and had an implementation before I realized I should have probably delegated it.
Please see if #124203 makes sense to you.

nohlson · 2024-09-24T01:47:49Z

@encukou I do prefer your PR that addresses the cause of the warning directly in unicodeobject.c #124203 either of the implementations. I will remove the changes here for unicodeobject.c and leave getversion.c changes.

encukou · 2024-09-25T17:56:07Z

That PR is merged now :)
But it seems new warnings appeared in one of the tests. You might be interested in __attribute__((format)), and how we apply it to e.g. PyOS_vsnprintf in Include/pyerrors.h.

Misc/NEWS.d/next/Security/2024-07-30-17-34-47.gh-issue-112301.8J8WhZ.rst

erlend-aasland · 2024-10-14T22:33:18Z

Tools/build/.warningignore_macos

+Objects/mimalloc/ *
+Python/pylifecycle.c 1
+Python/sysmodule.c 1
+Python/tracemalloc.c 1


Nit: can you please add the missing newline at EOF?

picnixz · 2024-10-22T12:10:48Z

@encukou

But it seems new warnings appeared in one of the tests. You might be interested in attribute((format)), and how we apply it to e.g. PyOS_vsnprintf in Include/pyerrors.h.

Is this still relevant? I stumbled upon that and wondered whether it's something relevant to add in this PR or in a separate PR (I was working with __attribute__ ((unused)) and remembered that there was also this attribute that could be useful).

nohlson added 2 commits July 30, 2024 11:42

Add -Wformat=2 compiler option to NODIST default

c5396a1

Add -Wformat to NODIST

19437ef

nohlson requested review from erlend-aasland and corona10 as code owners July 30, 2024 17:32

bedevere-app bot mentioned this pull request Jul 30, 2024

Consider applying flags for warnings about potential security issues #112301

Open

bedevere-app bot added the awaiting review label Jul 30, 2024

blurb-it bot and others added 2 commits July 30, 2024 17:34

📜🤖 Added by blurb_it.

cba84fe

Add fail on warning regression option to ubuntu builds

178bcc6

nohlson requested review from ezio-melotti and hugovk as code owners July 30, 2024 18:14

corona10 added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 31, 2024

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 31, 2024

nohlson commented Jul 31, 2024

View reviewed changes

configure.ac Show resolved Hide resolved

nohlson added 2 commits August 5, 2024 16:59

Add warning ignores to code with nonliteral format strings

3bc76ca

Limit pragmas to gcc/clang

c4b8cf9

encukou reviewed Aug 7, 2024

View reviewed changes

nohlson added 5 commits August 7, 2024 13:35

Refactor version _Py_InitVersion to use format string literals

8cff32e

Merge branch 'main' into add-format-level-2

95c612c

Create pyport preprocessor macro for ignoring format nonliterals

a3b0c46

Add MSVC boilerplate macro for ignoring format nonliteral

fa52862

Add nonliteral macro

6cd1def

Merge branch 'main' into add-format-level-2

13e9c80

nohlson added 3 commits September 30, 2024 14:15

Merge branch 'main' into add-format-level-2

b62d155

Merge main

9806f88

Pull unicodeobject changes

88fbc65

nohlson added 4 commits October 8, 2024 12:35

Merge branch 'main' into add-format-level-2

b6db214

Update macos warningignore for new warnings

304aed1

Fix mimalloc directory warning ignore

5afb8d8

Revert pyport

1e664f6

erlend-aasland reviewed Oct 14, 2024

View reviewed changes

Misc/NEWS.d/next/Security/2024-07-30-17-34-47.gh-issue-112301.8J8WhZ.rst Outdated Show resolved Hide resolved

Formatting nit

1b4089b

erlend-aasland reviewed Oct 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-112301: Add -Wformat=2 compiler option to NODIST #122474

gh-112301: Add -Wformat=2 compiler option to NODIST #122474

nohlson commented Jul 30, 2024 •

edited

Loading

bedevere-bot commented Jul 31, 2024

encukou Aug 7, 2024

nohlson Aug 7, 2024

encukou Aug 7, 2024

nohlson Sep 18, 2024

encukou commented Sep 18, 2024

nohlson commented Sep 24, 2024

encukou commented Sep 25, 2024

erlend-aasland Oct 14, 2024

picnixz commented Oct 22, 2024

gh-112301: Add -Wformat=2 compiler option to NODIST #122474

Are you sure you want to change the base?

gh-112301: Add -Wformat=2 compiler option to NODIST #122474

Conversation

nohlson commented Jul 30, 2024 • edited Loading

bedevere-bot commented Jul 31, 2024

encukou Aug 7, 2024

Choose a reason for hiding this comment

nohlson Aug 7, 2024

Choose a reason for hiding this comment

encukou Aug 7, 2024

Choose a reason for hiding this comment

nohlson Sep 18, 2024

Choose a reason for hiding this comment

encukou commented Sep 18, 2024

nohlson commented Sep 24, 2024

encukou commented Sep 25, 2024

erlend-aasland Oct 14, 2024

Choose a reason for hiding this comment

picnixz commented Oct 22, 2024

nohlson commented Jul 30, 2024 •

edited

Loading