Opened 11 months ago

Last modified 6 months ago

#26457 needs_info enhancement

Do not depend on a patched Python

Reported by: saraedum Owned by:
Priority: major Milestone: sage-wishlist
Component: distribution Keywords: conda, archlinux, gentoo, debian, nix, python, python3
Cc: gh-timokau, fbissey, arojas, embray, slelievre, nthiery, jdemeyer, isuruf, pcpa, thansen, infinity0, snark, cschwan Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

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.

Change History (23)

comment:1 Changed 11 months ago by saraedum

  • Status changed from new to needs_info

comment:2 Changed 11 months ago by saraedum

  • Cc isuruf added

comment:3 Changed 11 months ago by saraedum

  • Cc pcpa thansen infinity0 snark cschwan added

comment:4 follow-up: Changed 11 months ago by dimpase

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

Last edited 11 months ago by dimpase (previous) (diff)

comment:5 follow-up: Changed 11 months ago by gh-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.

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: https://github.com/python/cpython/pull/6118

The other fixes python bug #27177?, 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.

comment:6 follow-up: Changed 11 months ago by gh-timokau

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

comment:7 in reply to: ↑ 6 Changed 11 months ago by saraedum

Replying to gh-timokau:

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

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

comment:8 in reply to: ↑ 5 Changed 11 months ago by saraedum

Replying to gh-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.

comment:9 in reply to: ↑ 4 ; follow-up: Changed 11 months ago by 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.

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.)

Last edited 11 months ago by saraedum (previous) (diff)

comment:10 in reply to: ↑ 9 ; follow-ups: Changed 11 months ago by gh-timokau

Replying to saraedum:

Replying to gh-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 gh-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.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 11 months ago by saraedum

Replying to gh-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?

comment:12 in reply to: ↑ 10 ; follow-up: Changed 11 months ago by saraedum

Replying to gh-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.)

comment:13 in reply to: ↑ 11 ; follow-up: Changed 11 months ago by gh-timokau

Replying to saraedum:

Replying to gh-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).

comment:14 in reply to: ↑ 12 ; follow-up: Changed 11 months ago by gh-timokau

Replying to saraedum:

Replying to gh-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.

comment:15 in reply to: ↑ 13 Changed 11 months ago by saraedum

Replying to gh-timokau:

Replying to saraedum:

Replying to gh-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.

comment:16 in reply to: ↑ 14 ; follow-up: Changed 11 months ago by saraedum

Replying to gh-timokau:

Replying to saraedum:

Replying to gh-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.

comment:17 in reply to: ↑ 16 ; follow-up: Changed 11 months ago by jhpalmieri

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.

comment:18 in reply to: ↑ 17 ; follow-up: Changed 11 months ago by 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.

comment:19 in reply to: ↑ 18 ; follow-up: Changed 11 months ago by gh-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.

comment:20 in reply to: ↑ 19 ; follow-up: Changed 11 months ago by saraedum

Replying to gh-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.

comment:21 in reply to: ↑ 20 Changed 11 months ago by gh-timokau

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.

comment:22 Changed 11 months ago by embray

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

comment:23 Changed 6 months ago by embray

  • Milestone changed from sage-8.4 to sage-wishlist
Note: See TracTickets for help on using tickets.