Skip to content
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

src/sage/misc/latex.py: replace tmp_dir() #36436

Merged
merged 2 commits into from
Oct 21, 2023
Merged

Conversation

orlitzky
Copy link
Contributor

These bits of code only use one temporary file, but they call a function _run_latex_() that writes its output file to the same directory as its input file. Since we don't want the output path to be predictable (for security reasons), we have to "hide" the one temporary file inside of a temporary directory that will only be writable by the Sage user.

In other words: standard tempfile.TemporaryDirecrory() replacement.

Issue: #36322

These bits of code only use one temporary file, but they call a
function _run_latex_() that writes its output file to the same
directory as its input file. Since we don't want the output path to be
predictable (for security reasons), we have to "hide" the one
temporary file inside of a temporary directory that will only be
writable by the Sage user.

In other words: standard tempfile.TemporaryDirecrory() replacement.

Issue: sagemath#36322
Copy link
Member

@mezzarobba mezzarobba left a comment

Choose a reason for hiding this comment

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

Is there a reason not to use with TemporaryDirectory() (or a try-finally block) — especially in view, which has two possible exit points even in the absence of any exception?

This is less error-prone than calling cleanup() manually, although it
results in a much larger diff.
@orlitzky
Copy link
Contributor Author

Is there a reason not to use with TemporaryDirectory() (or a try-finally block) — especially in view, which has two possible exit points even in the absence of any exception?

The context manager is better but it makes the diff awful if you have to indent the entire method. I've made the change in a separate commit if you don't mind looking through it though.

@github-actions
Copy link

Documentation preview for this PR (built with commit 0c4578f; changes) is ready! 🎉

@jhpalmieri
Copy link
Member

@mezzarobba: do you have any remaining concerns? This looks good to me.

@mezzarobba
Copy link
Member

@mezzarobba: do you have any remaining concerns? This looks good to me.

No. I thought I had approved the changes, but apparently I made a mistake?

Copy link
Member

@mezzarobba mezzarobba left a comment

Choose a reason for hiding this comment

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

thanks!

vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 17, 2023
sagemathgh-36436: src/sage/misc/latex.py: replace tmp_dir()
    
These bits of code only use one temporary file, but they call a function
`_run_latex_()` that writes its output file to the same directory as its
input file. Since we don't want the output path to be predictable (for
security reasons), we have to "hide" the one temporary file inside of a
temporary directory that will only be writable by the Sage user.

In other words: standard `tempfile.TemporaryDirecrory()` replacement.

Issue: sagemath#36322
    
URL: sagemath#36436
Reported by: Michael Orlitzky
Reviewer(s): Marc Mezzarobba
@vbraun vbraun merged commit 4a60001 into sagemath:develop Oct 21, 2023
@mkoeppe mkoeppe added this to the sage-10.2 milestone Oct 21, 2023
@orlitzky orlitzky deleted the no-tmp-dir8 branch October 23, 2023 12:07
@EmmanuelCharpentier
Copy link
Contributor

This bungles view(). Highly damageable to console (or emacs' sage-shell-mode) users.

I opened #36526.

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.

6 participants