bpo-16575: Add checks for unions passed by value to functions. #16430

vsajip · 2019-09-26T16:48:11Z

https://bugs.python.org/issue16575

eliben · 2019-09-26T17:23:04Z

@vsajip long time not hear :) Sorry, I've been away for too long to be of much use here. Removing myself from the reviewer list

@encukou

…H-16248) As mentioned in the bpo ticket, this mistake came up on two reviews: - #16127 (review) - #16071 (review) Would be nice to have it documented in a more permanent place than 3.8's whatsnew entry. https://bugs.python.org/issue38206 Automerge-Triggered-By: @encukou

test_hmac and test_hashlib test built-in hashing implementations and OpenSSL-based hashing implementations. Add more checks to skip OpenSSL implementations when a strict crypto policy is active. Use EVP_DigestInit_ex() instead of EVP_DigestInit() to initialize the EVP context. The EVP_DigestInit() function clears alls flags and breaks usedforsecurity flag again. Signed-off-by: Christian Heimes <christian@python.org> https://bugs.python.org/issue38270

The "Slot" helper (descriptor) is leaking references due to its caching mechanism. The change includes a partial fix to Slot, but also adds Variable.storage to replace the problematic use of Slot. https://bugs.python.org/issue38187

Also cache the compiled RE for parsing the format specifier.

Escape the server title of xmlrpc.server.DocXMLRPCServer when rendering the document page as HTML.

…16446)

Add a new struct_size field to PyPreConfig and PyConfig structures to allow to modify these structures in the future without breaking the backward compatibility. * Replace private _config_version field with public struct_size field in PyPreConfig and PyConfig. * Public PyPreConfig_InitIsolatedConfig() and PyPreConfig_InitPythonConfig() return type becomes PyStatus, instead of void. * Internal _PyConfig_InitCompatConfig(), _PyPreConfig_InitCompatConfig(), _PyPreConfig_InitFromConfig(), _PyPreConfig_InitFromPreConfig() return type becomes PyStatus, instead of void. * Remove _Py_CONFIG_VERSION * Update the Initialization Configuration documentation.

…ion and encoding behavior (GH-16448) * bpo-38216: Allow bypassing input validation * bpo-36274: Also allow the URL encoding to be overridden. * bpo-38216, bpo-36274: Add tests demonstrating a hook for overriding validation, test demonstrating override encoding, and a test to capture expectation of the interface for the URL. * Call with skip_host to avoid tripping on the host checking in the URL. * Remove obsolete comment. * Make _prepare_path_encoding its own attr. This makes overriding just that simpler. Also, don't use the := operator to make backporting easier. * Add a news entry. * _prepare_path_encoding -> _encode_prepared_path() * Once again separate the path validation and request encoding, drastically simplifying the behavior. Drop the guarantee that all processing happens in _prepare_path.

Document that lnotab can contain invalid bytecode offsets (because of terrible reasons that are difficult to fix). Make dis.findlinestarts() ignore invalid offsets in lnotab. All other uses of lnotab in CPython (various reimplementations of addr2line or line2addr in Python, C and gdb) already ignore this, because they take an address to look for, instead. Add tests for the result of dis.findlinestarts() on wacky constructs in test_peepholer.py, because it's the easiest place to add them.

…ng. (GH-14389)

…H-16467)

vsajip · 2019-10-15T08:25:35Z

Sorry, all, for spamming with review requests. This happened because the PR branch had conflicts with master; I rebased with master and resolved conflicts, then pushed. I had no idea this would automatically add you all to the reviewer list.

vsajip · 2019-10-15T08:30:55Z

I'll close this and open another PR with just my changes.

gvanrossum · 2019-10-15T15:14:01Z

Vinay,

Thanks for explaining the steps in your workflow that caused this! This happens on a regular basis and I've always wondered what those uses are doing. Now I understand it better. But I still have a question, because I'm surprised that GitHub doesn't handle this better. Here's a hypothesis. IIRC there are two ways to rebase: rebase master onto your branch, or rebase your branch onto master. I always use the latter. Is that also what you did? IOW did the conflict resolution you had to do change some of your own commits for the PR, or did you have to change commits that came from master?

Maybe someone with a higher git wizard level can shed more light on this?

vsajip · 2019-10-15T16:33:48Z

I thought it would be wrong to rebase my branch onto master, as my branch is still work in progress and the master is supposed to be the best available development version, so shouldn't be polluted with my as-yet unfinalised code. I thought the rebase-branch-onto-master happens when the PR is merged, isn't that right?

For the conflict resolution, I just had to deal with some inserted code from another PR which confused Git's merge operation - so the conflict resolution involved both code which came from master and code from my PR. I barely did more than removing the conflict marker lines.

Perhaps the mistake was to do git rebase master rather than git merge master. With the rebase, it added commits from master onto my branch as if they had been there all along, with their various authors, then replayed my branch commits on top. Perhaps merging rather than rebasing would have been the right thing to do - there would just be a merge commit authored by me, so presumably no-one would have been spammed when I pushed the result. I aim to merge rather than rebase the next time a conflict with master arises, unless there's better advice from someone who's more expert in Git.

gvanrossum · 2019-10-15T16:58:12Z

Hm, `git rebase master` is what I would also have done. Maybe there was some discrepancy between what was in master in your private fork on GitHub compared to upstream?

methane · 2019-10-15T17:15:55Z

0b60f64 is in master branch.
I think this is a bug or limitation of Github. I saw same troubles many times.

gvanrossum · 2019-10-15T17:50:22Z

There's definitely something funky though. Looking at the commit log for the PR I see that the first five commits are Vinay's changes relevant to the PR; these are [bpo-16575: Add checks for unions passed by value to functions.](3796a4c) through Improved tests..

After that I see many other commits that were already on master. (These are the ones that caused everyone to be subscribed to the issue.)

Then at the very end I see the same five commits by Vinay again.

That's not what I would expect after a simple git rebase master: I would expect only a single copy of the relevant commits, at the end (after all the commits from master).

I still suspect that there was something funky involving Vinay's fork (which since has been updated so the history is not easily recovered). And I suspect that this is a simple trap in git behavior that many new contributors fall into as well. Maybe if we understand what happens better we can add advice to the devguide that will help people avoid this problem.

My theory is that it has something to do with synchronization between the master branch in the fork vs. the upstream master branch. But it would require experimentation to confirm, and I am out of time to play more with this.

terryjreedy · 2019-10-15T18:56:01Z

I am not a git expert so I never use rebase and instead always branch from local master (kept equal to origin/master) and update such branches as recommended on the original bootcamp page. After updating master, 'checkout branch' and 'merge origin/master'. The result is to add one squashed 'update merge' commit to the branch that is ignored when computing 'changed files', affected owners, and the displayed diff. Aside from dealing with merge conflicts, I have never seen update merges create a problem .

From reading 'help rebase', it seems to be aimed as doing more complicated alterations of the commit tree, such as rebasing a branch of a branch on master instead.

ncoghlan · 2019-10-15T20:59:22Z

Merging during review rather than rebasing is definitely the easiest way to avoid problems.

If folks do want to rebase, then "git rebase origin/master" (or "git rebase upstream/master" if your remotes are set up that way) can give more consistent results, as it avoids any discrepancies between the local master and the CPython repo master.

gvanrossum · 2019-10-15T22:48:10Z

Ah, I'm sure git rebase <upstream>/master is the key here! Thanks.

vsajip · 2019-10-16T04:01:59Z

But I generally effectively fetch upstream changes before anything else, so my local master should be synced to that of python/cpython on GitHub. It's possible I forgot to do this operation on this particular occasion - I can't be sure, as it's a muscle memory thing when I switch to doing CPython development. I use this script:

cd $HOME/projects/python/master
git fetch upstream
for BRANCH in master 3.8 3.7 3.6 2.7
do
  cd ../$BRANCH
  echo Syncing branch $BRANCH ...
  if [ $BRANCH == "master" ]; then
    # in case a fix branch was checked out
    git checkout $BRANCH
  fi
  git rebase upstream/$BRANCH
  git push origin $BRANCH
done

If anyone sees any drawbacks with this approach, please let me know.

gvanrossum · 2019-10-16T04:10:48Z

I wonder if you shouldn't also push upstream/master to origin.

terryjreedy · 2019-10-16T04:19:18Z

My .bat is essentially the same except for 'merge' instead of 'rebase'.

ncoghlan · 2019-10-16T04:22:40Z

The one potential risk I see with the script is that if you've accidentally committed to master before making a PR branch, then the script won't get rid of them, it will just give them new commit IDs.

Rebasing the PR branch on local master instead of upstream master would then have the potential to produce an unusual commit history.

Using "git reset" in your branch sync script instead of "git rebase" should avoid that risk.

vsajip · 2019-10-16T08:15:22Z

I wonder if you shouldn't also push upstream/master to origin.

But I am, in the script, in the line git push origin $BRANCH - I'm not sure what you mean.

methane · 2019-10-16T08:33:25Z

I suspect Github doesn't support "rebase origin/master; push -f" workflow in pull requests.
I reported this pull request to Github support.

gvanrossum · 2019-10-16T21:31:16Z

I use that workflow all the time in the mypy repo and this has never happened to me. I think Vinay may have had his commits in master by accident, and the git command may just have failed. If the Shell script doesn’t have -e it will just barrel on.

vsajip · 2019-10-17T04:35:26Z

I think Vinay may have had his commits in master by accident

I don't think so. I always work in a branch other than master. I have the Bash prompt set up to show me which branch I'm on, so it's hard to make a mistake.

Isn't this a case of the rebase operation working as expected, plus how GitHub handled it? That is, git started from where I originally branched, applied all new commits added to master since then, and then applied my original commits on top; I then added the resolving commit. When I pushed, GitHub then just concatenated all of these to my original commits (in terms of how it presents the PR), resulting in the above.

I plan to use merge rather than rebase from now on.

gvanrossum · 2019-10-17T04:54:40Z

I think Vinay may have had his commits in master by accident

I don't think so. I always work in a branch other than master. I have the Bash prompt set up to show me which branch I'm on, so it's hard to make a mistake.

OK, so much for that theory... :-)

Isn't this a case of the rebase operation working as expected, plus how GitHub handled it? That is, git started from where I originally branched, applied all new commits added to master since then, and then applied my original commits on top; I then added the resolving commit. When I pushed, GitHub then just concatenated all of these to my original commits (in terms of how it presents the PR), resulting in the above.

I've never seen git or GitHub double commits like that though. After a rebase, if you push without -f, the push will fail. With -f, the remote branch will look identical to the local branch. I still think something weird happened, and it just seems more likely that the cause was an unusual sequence of commands locally than an actual git or GitHub bug.

I plan to use merge rather than rebase from now on.

Sure. But we still don't know what sequence of commands caused this, and it will happen again to others.

Please understand I'm not picking on you or your git skills! I'm just nerd-sniped by a mystery that looks like it may have happened to others as well, and if so, will happen to yet others in the future, and I'd like to be able to warn people of the particular trap that led to this situation.

So here are two other theories.

First, you seem to be keeping each branch in its own directory. That's nice, but it differs from my workflow, so perhaps there's an explanation there. In fact I'm not sure how this is supposed to work. Are there symlinks? Or is there some magic in your .git/config file? It may be a common git setup that I'm just not familiar with. Come to think of it, where in this forest is the branch you use for your bugfix?

The other thing is that IIUC the typical advice for creating a branch is to use e.g. git checkout master -b fix-a-bug. My personal workflow however uses git checkout upstream/master -b fix-a-bug. Or in repos where I don't use a triangular workflow (e.g. because I'm the only developer) I use origin/master. This seems to work better with git fetch than branching directly off master.

Finally. Calling @gnprice, my go-to git wizard.

methane · 2019-10-17T05:40:48Z

I use that workflow all the time in the mypy repo and this has never happened to me.

My English bad. I didn't mean Github doesn't support rebase at all.
I use rebase + push -f workflow often too. It works 99% of time.

I just meant there is a bug or limitation in rebase support.

vsajip · 2019-10-17T10:53:31Z

Please understand I'm not picking on you or your git skills!

I'm not taking it that way! I'm as interested as you are in knowing what the root cause of the problem might be.

First, you seem to be keeping each branch in its own directory. That's nice, but it differs from my workflow, so perhaps there's an explanation there. In fact I'm not sure how this is supposed to work.

I'm using git worktree. I don't think there are symlinks, though there might be hard links. The multiple directories are there to facilitate builds only, so I can quickly test a script with multiple versions.

Come to think of it, where in this forest is the branch you use for your bugfix?

I work in the master subdirectory, but not in the master branch. I never make changes in the other subdirectories, and only configure and make there.

Starting from the master branch, I initially do checkout -b fix-NNNNN to start on a piece of work, switching back to the master branch only when pulling from upstream and building the latest upstream version. Generally all my work will be merged to master (on GitHub, from a PR), and then backported to other branches using the very useful miss-islington bot or cherry_picker on the command line if that fails. In such cases, I always work in the master subdirectory, but in the relevant backport branch.

Before doing a checkout -b fix-NNNNN, I always use the update script I listed above to ensure that my local master is synced with upstream/master.

I'd assumed what I do was a common workflow, but perhaps not!

ncoghlan · 2019-10-17T13:48:46Z

Your workflow is basically the same as mine (including the use of git worktree).

I've never managed to encounter the "duplicated commits after rebase" problem though, so I'm just as intrigued as everyone else as to how it actually comes about.

terryjreedy · 2019-10-17T18:13:20Z

I also use worktree directories, dev/38, etc, for 3.8, 3.7, and 2.7, which are updated with git merge after git fetch in dev/3x (master, now 3.9). I use them for compiling current 3.8, etc, and for manually creating branches and PRs from and for those Python version. (This is now very seldom). My dev/3x directory has a 250 MB .git directory. dev/38 has a .git file consisting of 'gitdir: f:/dev/3x/.git/worktrees/38'. Call it a 'link file'. So dev/38 has the 3.8 version of the cpython source tree, but git commands in that directory use the one 250 MB copy of the repository history and metadata kept in the master directory.

Worktrees are an alternative way to work with multiple python versions without waiting for hundreds of files to be changed, and then changed back again, and to do so simultaneously.

gvanrossum · 2019-10-17T21:40:12Z

Worktrees sound super useful! I've got to check them out.

methane · 2019-10-31T06:46:48Z

I get a reply from Github support. I shared it in committer's forum of the discuss.python.org.

https://discuss.python.org/t/info-rebase-origin-maseter-push-f-workflow-corrupts-pull-request-rarely/2558

vsajip added 5 commits September 26, 2019 12:06

bpo-16575: Add checks for unions passed by value to functions.

3796a4c

Updated the logic for propagation of flags.

f6f496a

Corrected typos in test.

dc31b8f

Removed debugging code.

b92f55a

Improved tests.

01ba08e

vsajip added type-bug An unexpected behavior, bug, or error skip news labels Sep 26, 2019

the-knights-who-say-ni added the CLA signed label Sep 26, 2019

bedevere-bot added the awaiting core review label Sep 26, 2019

vsajip requested a review from eliben September 26, 2019 17:19

vsajip self-assigned this Sep 26, 2019

vsajip requested review from amauryfa and abalkin September 26, 2019 17:21

eliben removed their request for review September 26, 2019 17:23

aixtools and others added 15 commits September 26, 2019 22:43

bpo-28009: Fix uuid.uuid1() and uuid.get_node() on AIX (GH-8672)

0bcbfa4

cleanup ababstractproperty in typing.py (GH-16432)

6ce03ec

bpo-32820: Simplify __format__ implementation for ipaddress. (GH-16378)

5d6f5b6

Also cache the compiled RE for parsing the format specifier.

Fix typo in the "Porting to Python 3.8" section. (GH-16435)

dd6117c

bpo-38243, xmlrpc.server: Escape the server_title (GH-16373)

e8650a4

Escape the server title of xmlrpc.server.DocXMLRPCServer when rendering the document page as HTML.

bpo-38301: In Solaris family, we must be sure to use '-D_REENTRANT' (#…

52d1b86

…16446)

bpo-37408: Precise that Tarfile "format" argument only concerns writi…

c5a7e0c

…ng. (GH-14389)

bpo-38310: Predict BUILD_MAP_UNPACK_WITH_CALL -> CALL_FUNCTION_EX. (G…

f185a73

…H-16467)

bpo-38108: Makes mock objects inherit from Base (GH-16060)

9a7d951

vsajip removed request for abalkin and 1st1 October 15, 2019 08:22

vsajip closed this Oct 15, 2019

vsajip deleted the fix-16575 branch October 15, 2019 08:31

arigo mannequin mentioned this pull request Apr 10, 2022

ctypes: unions as arguments #60779

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-16575: Add checks for unions passed by value to functions. #16430

bpo-16575: Add checks for unions passed by value to functions. #16430

vsajip commented Sep 26, 2019 •

edited by bedevere-bot

Loading

eliben commented Sep 26, 2019

vsajip commented Oct 15, 2019

vsajip commented Oct 15, 2019

gvanrossum commented Oct 15, 2019 •

edited

Loading

vsajip commented Oct 15, 2019

gvanrossum commented Oct 15, 2019 via email

methane commented Oct 15, 2019

gvanrossum commented Oct 15, 2019 •

edited by bedevere-bot

Loading

terryjreedy commented Oct 15, 2019

ncoghlan commented Oct 15, 2019

gvanrossum commented Oct 15, 2019

vsajip commented Oct 16, 2019

gvanrossum commented Oct 16, 2019 via email

terryjreedy commented Oct 16, 2019

ncoghlan commented Oct 16, 2019

vsajip commented Oct 16, 2019

methane commented Oct 16, 2019

gvanrossum commented Oct 16, 2019 •

edited

Loading

vsajip commented Oct 17, 2019

gvanrossum commented Oct 17, 2019

methane commented Oct 17, 2019

vsajip commented Oct 17, 2019

ncoghlan commented Oct 17, 2019

terryjreedy commented Oct 17, 2019

gvanrossum commented Oct 17, 2019

methane commented Oct 31, 2019 •

edited

Loading

bpo-16575: Add checks for unions passed by value to functions. #16430

bpo-16575: Add checks for unions passed by value to functions. #16430

Conversation

vsajip commented Sep 26, 2019 • edited by bedevere-bot Loading

eliben commented Sep 26, 2019

vsajip commented Oct 15, 2019

vsajip commented Oct 15, 2019

gvanrossum commented Oct 15, 2019 • edited Loading

vsajip commented Oct 15, 2019

gvanrossum commented Oct 15, 2019 via email

methane commented Oct 15, 2019

gvanrossum commented Oct 15, 2019 • edited by bedevere-bot Loading

terryjreedy commented Oct 15, 2019

ncoghlan commented Oct 15, 2019

gvanrossum commented Oct 15, 2019

vsajip commented Oct 16, 2019

gvanrossum commented Oct 16, 2019 via email

terryjreedy commented Oct 16, 2019

ncoghlan commented Oct 16, 2019

vsajip commented Oct 16, 2019

methane commented Oct 16, 2019

gvanrossum commented Oct 16, 2019 • edited Loading

vsajip commented Oct 17, 2019

gvanrossum commented Oct 17, 2019

methane commented Oct 17, 2019

vsajip commented Oct 17, 2019

ncoghlan commented Oct 17, 2019

terryjreedy commented Oct 17, 2019

gvanrossum commented Oct 17, 2019

methane commented Oct 31, 2019 • edited Loading

vsajip commented Sep 26, 2019 •

edited by bedevere-bot

Loading

gvanrossum commented Oct 15, 2019 •

edited

Loading

gvanrossum commented Oct 15, 2019 •

edited by bedevere-bot

Loading

gvanrossum commented Oct 16, 2019 •

edited

Loading

methane commented Oct 31, 2019 •

edited

Loading