Do not depend on a patched Python

[saraedum]

SageMath ships a patched Python which adds a warning if . is on the path and deemed insecure, see https://bugs.python.org/issue16202 and #13579.

This patch is not going to be part of Python anytime soon and afaik no distribution other than Sage-the-distribution is shipping this patch.

For packagers it is annoying to work around the doctests that expect this Python patch to be applied. Also it means that for Sage development we cannot swap out SageMath's Python with one coming from our (Linux) distribution. But long-term we want to be able to replace more SPKGs with distribution packages. (I know that embray is working on that.)

I would like to discuss this issue here. From my point of view, we should not expect this patch to be applied in doctesting and in Sage in general. (Actually, I don't think we should apply this patch at all and just behave like vanilla Python does but that's probably too controversial.)

I would like to hear some arguments here and come up with something that is going to make packagers happy because I believe that's really what we should be striving for. We might want a vote on sage-devel in the end but that remains to be seen.

I hope I am not creating a duplicate with this ticket (I might have tried to do something like that before...)

See #25358 for a related ticket.

CC: @timokau @kiwifb @antonio-rojas @embray @slel @nthiery @jdemeyer @isuruf @pcpa @tobihan @infinity0 @SnarkBoojum @cschwan

Component: distribution

Keywords: conda, archlinux, gentoo, debian, nix, python

Issue created by migration from https://trac.sagemath.org/ticket/26457

[dimpase]

comment:4

we can simply tag these tests, and then in the unpatched python case they won't be run.

[timokau]

comment:5

Your description sounds like a duplicate of #25358, but if I read the title correctly you want to go further and also remove all other patches? I agree with that in principle, but it will be hard.

We currently apply two patches to python that I added just for sage. One is a bug that has since been fixed upstream and will not be necessary once there is a new release: python/cpython#6118

The other fixes [[(https://bugs.python.org/issue27177]], which was fixed but never backported. I have not found any way to work around this patch since we need re to work with sage integers but re is not (at least not easily) monkey-patchable.

[timokau]

comment:6

I fully agree about the safe-directory part of course.

[saraedum]

comment:7

Replying to @timokau:

I fully agree about the safe-directory part of course.

Could you clarify what you agree to? To drop the patches?

[saraedum]

comment:8

Replying to @timokau:

Your description sounds like a duplicate of #25358, but if I read the title correctly you want to go further and also remove all other patches? I agree with that in principle, but it will be hard.

Yes, I should have been more clear about that. I want to do something about the patches that won't make it into upstream Python 2 because these will also never make it into the majority of distributions. I wasn't aware of the "re" one.

[saraedum]

comment:9

Replying to @dimpase:

we can simply tag these tests, and then in the unpatched python case they won't be run.

So, something like # optional: sage-the-distribution. That would work for packagers I guess.

I wonder whether that Python patch could not be monkey-patched in Sage somehow and then everybody would be happy? (We can of course add it for the doctesting as was done in #25358 but that does not solve the general problem of running Sage in an insecure location.)

[timokau]

comment:10

Replying to @saraedum:

Replying to @timokau:

I fully agree about the safe-directory part of course.

Could you clarify what you agree to? To drop the patches?

I'd be fine with dropping or keeping the patch, as long as the doctests don't depend on it.

Replying to @saraedum:

Replying to @timokau:

Your description sounds like a duplicate of #25358, but if I read the title correctly you want to go further and also remove all other patches? I agree with that in principle, but it will be hard.

Yes, I should have been more clear about that. I want to do something about the patches that won't make it into upstream Python 2 because these will also never make it into the majority of distributions. I wasn't aware of the "re" one.

The re one is hard. I think it is technically possible to monkey patch re, but it does require selling the soul of your firstborn since it is a builtin: https://gist.github.com/mahmoudimus/295200

Other than that we could try to push upstream for a backport once again. Their reasoning is that the fix actually adds a feature instead of fixing a bug and only bugfixes are backported.

Replying to @saraedum:

Replying to @dimpase:

we can simply tag these tests, and then in the unpatched python case they won't be run.

So, something like # optional: sage-the-distribution. That would work for packagers I guess.

We would have to be very careful with that, too many differences could make sage-on-distro a second class citizen. Also it would probably be tempting to take the easy way out and "fix" problems by disabling tests on distro instead of fixing the underlying issue. On very limited cases like this it would probably be fine.

[saraedum]

comment:11

Replying to @timokau:

Replying to @saraedum:

So, something like # optional: sage-the-distribution. That would work for packagers I guess.

We would have to be very careful with that, too many differences could make sage-on-distro a second class citizen. Also it would probably be tempting to take the easy way out and "fix" problems by disabling tests on distro instead of fixing the underlying issue. On very limited cases like this it would probably be fine.

I agree. So could we change the underlying problem here? You demonstrated that we can check sys.path when doctesting on an unpatched Sage. Can't we do the same for normal invocations of Sage and then drop that patch to Python?

[saraedum]

comment:12

Replying to @timokau:

The re one is hard. I think it is technically possible to monkey patch re, but it does require selling the soul of your firstborn since it is a builtin: https://gist.github.com/mahmoudimus/295200

I didn't know about this stuff…that's horrifying :)

Other than that we could try to push upstream for a backport once again. Their reasoning is that the fix actually adds a feature instead of fixing a bug and only bugfixes are backported.

Could we just change Sage and it's doctests to not to rely on this? (and keep the patch in the Sage-the-distribution.)

[timokau]

comment:13

Replying to @saraedum:

Replying to @timokau:

Replying to @saraedum:

So, something like # optional: sage-the-distribution. That would work for packagers I guess.

We would have to be very careful with that, too many differences could make sage-on-distro a second class citizen. Also it would probably be tempting to take the easy way out and "fix" problems by disabling tests on distro instead of fixing the underlying issue. On very limited cases like this it would probably be fine.

I agree. So could we change the underlying problem here? You demonstrated that we can check sys.path when doctesting on an unpatched Sage. Can't we do the same for normal invocations of Sage and then drop that patch to Python?

We could just call the re-written test_safe_directory (with which Jeroen wasn't quite happy yet) at sage startup (before importing anything to avoid running in the security issue).

[timokau]

comment:14

Replying to @saraedum:

Replying to @timokau:

The re one is hard. I think it is technically possible to monkey patch re, but it does require selling the soul of your firstborn since it is a builtin: https://gist.github.com/mahmoudimus/295200

I didn't know about this stuff…that's horrifying :)

Other than that we could try to push upstream for a backport once again. Their reasoning is that the fix actually adds a feature instead of fixing a bug and only bugfixes are backported.

Could we just change Sage and it's doctests to not to rely on this? (and keep the patch in the Sage-the-distribution.)

Being able use re and access matches seems pretty essential to me, just removing the doctests wouldn't solve the problem.

[saraedum]

comment:15

Replying to @timokau:

Replying to @saraedum:

Replying to @timokau:

Replying to @saraedum:

So, something like # optional: sage-the-distribution. That would work for packagers I guess.

We would have to be very careful with that, too many differences could make sage-on-distro a second class citizen. Also it would probably be tempting to take the easy way out and "fix" problems by disabling tests on distro instead of fixing the underlying issue. On very limited cases like this it would probably be fine.

I agree. So could we change the underlying problem here? You demonstrated that we can check sys.path when doctesting on an unpatched Sage. Can't we do the same for normal invocations of Sage and then drop that patch to Python?

We could just call the re-written test_safe_directory (with which Jeroen wasn't quite happy yet) at sage startup (before importing anything to avoid running in the security issue).

That sounds like a good plan to me. Let me have a look at this tomorrow.

[saraedum]

comment:16

Replying to @timokau:

Replying to @saraedum:

Replying to @timokau:

The re one is hard. I think it is technically possible to monkey patch re, but it does require selling the soul of your firstborn since it is a builtin: https://gist.github.com/mahmoudimus/295200

I didn't know about this stuff…that's horrifying :)

Other than that we could try to push upstream for a backport once again. Their reasoning is that the fix actually adds a feature instead of fixing a bug and only bugfixes are backported.

Could we just change Sage and it's doctests to not to rely on this? (and keep the patch in the Sage-the-distribution.)

Being able use re and access matches seems pretty essential to me, just removing the doctests wouldn't solve the problem.

I don't think that I've ever used re in Sage. I think it's not really important in a math context. I think we could change the few doctests where we actually access a match with a Sage Integer to use named groups instead; and explain why that might be a better idea in the doctest. However, I have not had a look at these tests yet.

[jhpalmieri]

comment:17

Replying to @saraedum:

I don't think that I've ever used re in Sage.

Okay

I think it's not really important in a math context.

First, I hope you're not just generalizing from your own experience: that is a dangerous thing to do. Second, it is used in the Sage library, for example for argument parsing or LaTeX stuff. And therefore, third, Sage is not just a math context; people use it for other things, such as developing Sage. When working on Sage development, I often have used re, and to be honest, until Sage was patched, it took me a long time to understand why m = re.match(...) seemed to work, but m.group(0) did not. I'm an experienced Sage user, but using m.group(int(0)) was not obvious. If we just remove support for Sage integers in the group method, we will end up with some confused users.

So a strong -1 for removing this patch.

[saraedum]

comment:18

Replying to @jhpalmieri:

[...]
So a strong -1 for removing this patch.

I did not mean to remove this patch:

Could we just change Sage and it's doctests to not to rely on this? (and keep the patch in the Sage-the-distribution.)

I want to remove reliance on this patch from the Sage library so things work without it and change the doctests that rely on it/change them to #optional: python3.

[timokau]

comment:19

Replying to @saraedum:

Replying to @jhpalmieri:

[...]
So a strong -1 for removing this patch.

I did not mean to remove this patch:

Could we just change Sage and it's doctests to not to rely on this? (and keep the patch in the Sage-the-distribution.)

I want to remove reliance on this patch from the Sage library so things work without it and change the doctests that rely on it/change them to #optional: python3.

That is going in the direction of "distribution of second class citizen", with a feature (not a small one in my opinion) only working/tested on sage-the-distribution. I don't think removing a test for a valid use case is a good idea. At least I see value in that test for the nix package.

[saraedum]

comment:20

Replying to @timokau:

Replying to @saraedum:

Replying to @jhpalmieri:

[...]
So a strong -1 for removing this patch.

I did not mean to remove this patch:

Could we just change Sage and it's doctests to not to rely on this? (and keep the patch in the Sage-the-distribution.)

I want to remove reliance on this patch from the Sage library so things work without it and change the doctests that rely on it/change them to #optional: python3.

That is going in the direction of "distribution of second class citizen", with a feature (not a small one in my opinion) only working/tested on sage-the-distribution. I don't think removing a test for a valid use case is a good idea. At least I see value in that test for the nix package.

In this case, I don't actually see this problem. We would be testing this for Python 3 at least which is where we are going to anyway. (We could of course add an additional test for optional: sage-the-distribution.)

I might be wrong but I can't see that we could get the re patch into Debian or Conda (and I wouldn't even propose to try.) So, should Sage's documentation not be clear about the fact that this is a feature that may or may not work for you depending on how you are running Sage exactly? Doing a optional: python3 therefore seems very reasonable to me.

[timokau]

comment:21

In this case, I don't actually see this problem. We would be testing this for Python 3 at least which is where we are going to anyway. (We could of course add an additional test for optional: sage-the-distribution.)

I think python3 is still going to take some time.

I might be wrong but I can't see that we could get the re patch into Debian or Conda (and I wouldn't even propose to try.) So, should Sage's documentation not be clear about the fact that this is a feature that may or may not work for you depending on how you are running Sage exactly? Doing a optional: python3 therefore seems very reasonable to me.

Are you sure? Its a very reasonable two line patch that has pretty much no backwards incompatibility problems and has been accepted for 3.x: https://bugs.python.org/file43084/re_match_index.patch

Having regex not work would be confusing for users.

[embray]

comment:22

Haven't read all the discussion yet, but definitely +1.

[slel]

comment:24

Where does this stand with Python 2 support dropped?

[kiwifb]

comment:25

Well, I am using python 3.7 and 3.8 from the dsitro in sage-on-gentoo, without needing additional patches. Prior to the move to python3 I needed to provide a patched python 2.7 for sage-on-gentoo.

Not anymore.

I'd say it is obsolete.

[jhpalmieri]

comment:26

Most of the existing patches look like they're cygwin-related. Unless I'm misreading things, linux_linking_issue_25229.patch is the one exception, but I don't know what it's for. I guess it comes from https://bugs.python.org/issue25229. Looks like it might be merged soon?

(And I know nothing about cygwin, so I don't know if those patches are still necessary.)

[kiwifb]

comment:27

Replying to @jhpalmieri:

Most of the existing patches look like they're cygwin-related. Unless I'm misreading things, linux_linking_issue_25229.patch is the one exception, but I don't know what it's for. I guess it comes from https://bugs.python.org/issue25229. Looks like it might be merged soon?

(And I know nothing about cygwin, so I don't know if those patches are still necessary.)

I know nothing about cygwin either. I do remember pottering with a linking issue when using clang on linux in some cases. So, I am probably responsible for bringing that one in. Distros may very well already deal with that particular one.

[mkoeppe]

comment:28

I think this ticket can be closed as outdated.