Skip to content

gh-129594: Remove redundant check on varargs in _PyArg_CheckPositional #129595

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 3 commits into from
May 26, 2025

Conversation

eendebakpt
Copy link
Contributor

@eendebakpt eendebakpt commented Feb 2, 2025

@eendebakpt eendebakpt requested a review from picnixz April 4, 2025 13:42
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is indeed unnecessary but can you transform it into an assertion? just so that we're sure we don't call the function incorrectly or is it impossible to add some assertion inside this?

@eendebakpt
Copy link
Contributor Author

I think we could use an assertion within the macro. But if there is no reason to have it, it can only cause confusion.

The code in question was moved around a bit by Victor Stinner (only refactoring, functionality was not touched afaics).
The check was introduced in https://github.com/python/cpython/pull/18609/files (under a slightly different name ANY_VARARGS). From that PR it is not clear to me why it was added. @isidentical As PR author do you know why the check was added?

@eendebakpt eendebakpt requested a review from picnixz May 23, 2025 21:22
@picnixz picnixz self-assigned this May 23, 2025
@picnixz
Copy link
Member

picnixz commented May 23, 2025

I'll merge this one tomorrow, sorry for forgetting about this one :')

@picnixz
Copy link
Member

picnixz commented May 24, 2025

The PY_SSIZE_T_MAX check might be because of argument clinic:

static PyObject *
sys_audit(PyObject *module, PyObject *const *args, Py_ssize_t nargs)
{
    PyObject *return_value = NULL;
    const char *event;
    PyObject *__clinic_args = NULL;

    if (!_PyArg_CheckPositional("audit", nargs, 1, PY_SSIZE_T_MAX)) {
        goto exit;
    }

In the situation above, we want to check that nargs is at least 1. However, because of the != PY_SSIZE_T_MAX, we're actually making a real call to _PyArg_CheckPositional even in the success case, which is not ideal.

So I don't think we need the check at all as it doesn't help us (and even slows us down when sys.audit("abc") is correct)

@picnixz
Copy link
Member

picnixz commented May 25, 2025

arf, I forgot to merge this one. @eendebakpt can you confirm my analysis? I may have done it while I was just waking up and now I'm about to sleep :') (sorry, tomorrow it will be merged!)

@eendebakpt
Copy link
Contributor Author

Don't ask for analysis and promise to merge tomorrow, you might end up in an awkward situation ;-)

I agree with your analysis. The PY_SSIZE_T_MAX is a signal from argument clinic when have a variable number of arguments (e.g. *args). This is defined here:

NO_VARARG: Final[str] = "PY_SSIZE_T_MAX"

max_args = NO_VARARG if self.varpos else self.max_pos

@picnixz
Copy link
Member

picnixz commented May 26, 2025

Ok thanks.

To summarize _PyArg_CheckPositional(funcname, n, 1, PY_SSIZE_T_MAX) == 1 for n < PY_SSIZE_T_MAX and _PyArg_CheckPositional(funcname, PY_SSIZE_T_MAX, 1, PY_SSIZE_T_MAX) == 1 required an additional function call before this change, while now they can just use the (min) <= (nargs) && (nargs) <= (max) check.

@picnixz picnixz merged commit fb09db1 into python:main May 26, 2025
45 checks passed
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot aarch64 Android 3.x (tier-3) has failed when building commit fb09db1.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1594/builds/2507) and take a look at the build logs.
  4. Check if the failure is related to this commit (fb09db1) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/1594/builds/2507

Summary of the results of the build (if available):

Click to see traceback logs
remote: Enumerating objects: 9, done.        
remote: Counting objects:  11% (1/9)        
remote: Counting objects:  22% (2/9)        
remote: Counting objects:  33% (3/9)        
remote: Counting objects:  44% (4/9)        
remote: Counting objects:  55% (5/9)        
remote: Counting objects:  66% (6/9)        
remote: Counting objects:  77% (7/9)        
remote: Counting objects:  88% (8/9)        
remote: Counting objects: 100% (9/9)        
remote: Counting objects: 100% (9/9), done.        
remote: Compressing objects:  12% (1/8)        
remote: Compressing objects:  25% (2/8)        
remote: Compressing objects:  37% (3/8)        
remote: Compressing objects:  50% (4/8)        
remote: Compressing objects:  62% (5/8)        
remote: Compressing objects:  75% (6/8)        
remote: Compressing objects:  87% (7/8)        
remote: Compressing objects: 100% (8/8)        
remote: Compressing objects: 100% (8/8), done.        
remote: Total 9 (delta 2), reused 1 (delta 1), pack-reused 0 (from 0)        
From https://github.com/python/cpython
 * branch                    main       -> FETCH_HEAD
Note: switching to 'fb09db1b934a51e52e46dc64d7e5e84fe69e6160'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at fb09db1b934 gh-129594: Remove redundant check on varargs in `_PyArg_CheckPositional` (#129595)
Switched to and reset branch 'main'

configure: WARNING: no system libmpdecimal found; falling back to bundled libmpdecimal (deprecated and scheduled for removal in Python 3.15)
configure: WARNING: pkg-config is missing. Some dependencies may not be detected correctly.

  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0

100  192k  100  192k    0     0   255k      0 --:--:-- --:--:-- --:--:--  255k
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0

100 42455  100 42455    0     0  93544      0 --:--:-- --:--:-- --:--:-- 93544
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0

100 5041k  100 5041k    0     0  7181k      0 --:--:-- --:--:-- --:--:-- 7181k
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0

100 1252k  100 1252k    0     0  1520k      0 --:--:-- --:--:-- --:--:-- 1520k
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0

100  635k  100  635k    0     0  1841k      0 --:--:-- --:--:-- --:--:-- 1841k
../../configure: line 4071: pkg-config: command not found
configure: WARNING: no system libmpdecimal found; falling back to bundled libmpdecimal (deprecated and scheduled for removal in Python 3.15)
configure: WARNING: pkg-config is missing. Some dependencies may not be detected correctly.

../../Python/fileutils.c:460:1: warning: unused function 'decode_current_locale' [-Wunused-function]
  460 | decode_current_locale(const char* arg, wchar_t **wstr, size_t *wlen,
      | ^~~~~~~~~~~~~~~~~~~~~
../../Python/fileutils.c:679:1: warning: unused function 'encode_current_locale' [-Wunused-function]
  679 | encode_current_locale(const wchar_t *text, char **str,
      | ^~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
../../Modules/_localemodule.c:148:1: warning: unused function 'is_all_ascii' [-Wunused-function]
  148 | is_all_ascii(const char *str)
      | ^~~~~~~~~~~~
1 warning generated.
../../Modules/_hacl/Lib_Memzero0.c:66:6: warning: "Your platform does not support any safe implementation of memzero -- consider a pull request!" [-W#warnings]
   66 |     #warning "Your platform does not support any safe implementation of memzero -- consider a pull request!"
      |      ^
1 warning generated.

  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100  8784  100  8784    0     0  89466      0 --:--:-- --:--:-- --:--:-- 89632
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:01 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:02 --:--:--     0
  0    22    0     0    0     0      0      0 --:--:--  0:00:02 --:--:--     0
curl: (56) The requested URL returned error: 429
Warning: Problem (retrying all errors). Will retry in 1 seconds. 5 retries 
Warning: left.

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:01 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:02 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:03 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:04 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:05 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:06 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:07 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:08 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:09 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:10 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:12 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:13 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:14 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:15 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:16 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:17 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:18 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:19 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:20 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:22 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:23 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:24 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:25 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:26 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:27 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:28 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:29 --:--:--     0
  0   451    0     0    0     0      0      0 --:--:--  0:00:30 --:--:--     0
curl: (56) The requested URL returned error: 503
Warning: Problem (retrying all errors). Will retry in 2 seconds. 4 retries 
Warning: left.

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100  2894  100  2894    0     0  39305      0 --:--:-- --:--:-- --:--:-- 39643
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:01 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:02 --:--:--     0
  0    22    0     0    0     0      0      0 --:--:--  0:00:02 --:--:--     0
curl: (56) The requested URL returned error: 429
Warning: Problem (retrying all errors). Will retry in 1 seconds. 5 retries 
Warning: left.

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:01 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:02 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:03 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:04 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:05 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:06 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:07 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:08 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:09 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:11 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:12 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:13 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:14 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:15 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:16 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:17 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:18 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:20 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:21 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:22 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:23 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:24 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:25 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:26 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:27 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:29 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:30 --:--:--     0
  0   452    0     0    0     0      0      0 --:--:--  0:00:30 --:--:--     0
curl: (56) The requested URL returned error: 503
Warning: Problem (retrying all errors). Will retry in 2 seconds. 4 retries 
Warning: left.

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:01 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:02 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:03 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:04 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:05 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:06 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:07 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:08 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:09 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:10 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:11 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:13 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:14 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:15 --:--:--     0
  0   452    0     0    0     0      0      0 --:--:--  0:00:15 --:--:--     0
curl: (56) The requested URL returned error: 503
Warning: Problem (retrying all errors). Will retry in 4 seconds. 3 retries 
Warning: left.

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:01 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:02 --:--:--     0
  0    22    0     0    0     0      0      0 --:--:--  0:00:02 --:--:--     0
curl: (56) The requested URL returned error: 429
Warning: Problem (retrying all errors). Will retry in 8 seconds. 2 retries 
Warning: left.

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:01 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:02 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:03 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:04 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:05 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:06 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:07 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:08 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:10 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:11 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:12 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:13 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:14 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:15 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:16 --:--:--     0
  0    22    0     0    0     0      0      0 --:--:--  0:00:17 --:--:--     0
curl: (56) The requested URL returned error: 429
Warning: Problem (retrying all errors). Will retry in 16 seconds. 1 retries 
Warning: left.

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:01 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:02 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:03 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:04 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:05 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:06 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:07 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:08 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:09 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:10 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:12 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:13 --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:14 --:--:--     0
  0   452    0     0    0     0      0      0 --:--:--  0:00:15 --:--:--     0
curl: (56) The requested URL returned error: 503

@mhsmith
Copy link
Member

mhsmith commented May 26, 2025

This was caused by a general GitHub outage: https://www.githubstatus.com/incidents/d0nm3xcdc5jw

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants