Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#15515 closed enhancement (fixed)

Change graphics_filename() to return a tmp_filename() except from the notebook

Reported by: jdemeyer Owned by:
Priority: minor Milestone: sage-6.3
Component: misc Keywords:
Cc: Merged in:
Authors: Jeroen Demeyer Reviewers: Martin von Gagern
Report Upstream: N/A Work issues:
Branch: 69b285d (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

The function graphics_filename() in sage/misc/temporary_file.py was created for the notebook. When used in other places, it is unsafe (race condition + creates predicatable filenames) and can clutter the current working directory with files. Therefore, we should use tmp_filename() in graphics_filename() except when EMBEDDED_MODE is True. This also simplifies the logic in various show() methods.

Attachments (1)

doctest_no_graphics_filename.patch (12.7 KB) - added by jdemeyer 7 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 7 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Assert that graphics_filename() is only used in the notebook to Change graphics_filename() to return a tmp_filename() when doctesting

comment:2 Changed 7 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Change graphics_filename() to return a tmp_filename() when doctesting to Change graphics_filename() to return a tmp_filename() except from the notebook

comment:3 Changed 7 years ago by jdemeyer

  • Description modified (diff)

Changed 7 years ago by jdemeyer

comment:4 Changed 7 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/15515
  • Created changed from 12/12/13 16:47:53 to 12/12/13 16:47:53
  • Modified changed from 12/12/13 17:30:02 to 12/12/13 17:30:02

comment:5 Changed 7 years ago by jdemeyer

  • Status changed from new to needs_review

comment:6 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:7 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:8 Changed 7 years ago by rws

  • Commit set to 43137d0eb054bc5e7a46659e5693fcc1376455b9
  • Status changed from needs_review to needs_work
  • Work issues set to rebase

New commits:

43137d0graphics_filename: return a tmp_filename() if not in EMBEDDED_MODE

comment:9 Changed 7 years ago by jdemeyer

rws, do you care about this patch, would you review it? I can rebase the patch, but only if I know that somebody is interested in reviewing it.

comment:10 Changed 7 years ago by rws

No, I don't care about the notebook.

comment:11 Changed 7 years ago by rws

Oh it's not the notebook. Yes, I do care.

comment:12 follow-up: Changed 7 years ago by gagern

  • Branch changed from u/jdemeyer/ticket/15515 to u/gagern/ticket/15515
  • Commit changed from 43137d0eb054bc5e7a46659e5693fcc1376455b9 to b3e656b4507d9f6544e87641614836dc6275e929
  • Status changed from needs_work to needs_review
  • Work issues rebase deleted

Rebased this. Funny, I was coding something along the same lines at the moment, before comment:34:ticket:16533 made me aware of this ticket here.

My own modifications (which I have just modified to build on this here) also move some other tasks into that function, namely passing a caller-provided filename back (either unaltered or with added suffix) unless it is None. That way, one can simply do graphics_filename(filename=filename) without worrying whether the user specified a file name or not.

But since I might end up reviewing your commit, I probably should file my own as a separate ticket… I haven't looked at all your modifications yet, but I notice that code in plot3d/base.pyx is still pretty broken with regard to graphics_filename: they call it to generate some name, then strip the original extension and add an assortment of other extensions. That's just asking for a name clash. And if SAGE_TMP would happen to be world writable (although I can't imagine how that might be), then a security relevant race condition would be very likely.

If I were to fix this, I would be using my own code for this. Jeroen, could we review one another's code for this?

I guess I'd also like to use this function (with my own additions) all over the place for animations. Doing that would fix #16608 as well, would be a useful basis for my next stab at #16533, and would make 005b83f from #16571 superfluous as well.


New commits:

b3e656bgraphics_filename: return a tmp_filename() if not in EMBEDDED_MODE

comment:13 in reply to: ↑ 12 ; follow-up: Changed 7 years ago by jdemeyer

Replying to gagern:

If I were to fix this, I would be using my own code for this. Jeroen, could we review one another's code for this?

Yes, but not before the end of July. Please remind me then in case I forget.

comment:14 Changed 7 years ago by gagern

I had a closer look at the code. Decided to restore explicit arguments in two cases because they might have been used as positional arguments, not keyword arguments, so we'd break backward compatibility. This is in response to the discussion staring at comment:25:ticket:16533. If we want to deprecate them properly, I'd suggest doing so along the lines of #16607.

Apart from that, the changes all look good. I'm doing the build, doc build, long test suite and so on tests now, and will push this change and set positive review once I'm done, unless any problems pop up.

comment:15 Changed 7 years ago by gagern

I just filed #16640 for the Graphics3d problem, since that may be harder to fix, but is already a problem without this change here applied to it, so it should be dealt with independently. Once it has been dealt with, that code can also be changed to use graphics_filename in all cases, instead of only the embedded case.

comment:16 Changed 7 years ago by git

  • Commit changed from b3e656b4507d9f6544e87641614836dc6275e929 to 69b285d7fb1ebf189aea0b7cde311880ec884d6d

Branch pushed to git repo; I updated commit sha1. New commits:

69b285dRestore possibly positional arguments.

comment:17 Changed 7 years ago by gagern

  • Reviewers set to Martin von Gagern
  • Status changed from needs_review to positive_review

OK, now even the old API with positional parameters is restored. Everything looks good: look at the code, interactive experiments, doctests, documentation build. I hope it's OK if I give this a positive review even though the last commit is mine. After all, its a minor change, mostly restoring previous code.

comment:18 Changed 7 years ago by vbraun

  • Branch changed from u/gagern/ticket/15515 to 69b285d7fb1ebf189aea0b7cde311880ec884d6d
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:19 in reply to: ↑ 13 Changed 7 years ago by gagern

  • Commit 69b285d7fb1ebf189aea0b7cde311880ec884d6d deleted

Replying to jdemeyer:

Jeroen, could we review one another's code for this?

Yes, but not before the end of July. Please remind me then in case I forget.

Jeroen, can you please have a look at #16640? Of course I'll welcome reviews for all my other modifications as well, but that ticket is closely related to what you did here.

Note: See TracTickets for help on using tickets.