Graphics3d.show abuses graphics_filename

[gagern]

Code in sage/plot/plot3d/base.pyx in the method Graphics3d.show() makes invalid use of graphics_filename: it generates a name (like sage0.png), then strips the extension and appends various other extensions during the course of the method. But graphics_filename makes no guarantees that these other file names will be unused. In fact chances are pretty good that these files already exist from a different run of that method inside the same cell. So we get clashes there. Perhaps if things change in other places we might even get a security issue due to a race condition here.

I can think of two possible approaches: either call graphics_filename in several places, once for every file name we need to generate. Or create a temporary directory and write all files inside that with fixed names. The former will likely be easier to handle for notebook, but may make the code harder to read. Particularly those cases where some tool will use one file name to derive another file name might be a real pain, like when the applet size is encoded in the file name, or the magic aroud the archive_name for jmol. Will need some good interactive testing to make sure that everything still works after these changes.

Component: graphics

Author: Martin von Gagern, Jeroen Demeyer

Branch/Commit: 034fa58

Reviewer: Jeroen Demeyer, Karl-Dieter Crisman

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

[gagern]

Branch: u/gagern/ticket/16640

[gagern]

comment:2

Note that only the last two commits are new for this ticket; the rest comes from #16645 and its dependencies.

I tested things both in notebook and interactively. java3d failed in both cases, for which I just filed #16647. Apart from that, everything looks fine. In particular it seems as if I got all the file name interdependencies for jmol right.

---

New commits:

58b9c46	minimal fix to restore functionality
16cbd2d	Merge branch 'develop' into ticket/16533-stopgap
b3e656b	graphics_filename: return a tmp_filename() if not in EMBEDDED_MODE
69b285d	Restore possibly positional arguments.
e3c5542	Merge branch ticket/15515 into ticket/16608 to create ticket/16645.
9f30e94	Always use graphics_filename instead of tmp_filename.
bb7307f	Prevent doctest from leaking file into current working directory.
0dffab6	Include dot in extension for graphics_filename().
9dfe133	Generate names for 3d graphics files independently.

[gagern]

Author: Martin von Gagern

[gagern]

Dependencies: #16645

[gagern]

Commit: 9dfe133

[gagern]

comment:3

These are the changes that need reviewing in this ticket here.

[sagetrac-git]

Changed commit from 9dfe133 to cf63c2d

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

301185b	Include dot in extension for graphics_filename().
cf63c2d	Generate names for 3d graphics files independently.

[gagern]

Changed dependencies from #16645 to none

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

61809d6	Include dot in extension for graphics_filename().
84a2f30	Generate names for 3d graphics files independently.

[sagetrac-git]

Changed commit from cf63c2d to 84a2f30

[jdemeyer]

Changed branch from u/gagern/ticket/16640 to u/jdemeyer/ticket/16640

[sagetrac-git]

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

1a48c6b	Use "with open() as f" syntax
49ce20b	Set fg to figsize[0] like before

[sagetrac-git]

Changed commit from 84a2f30 to 49ce20b

[jdemeyer]

Reviewer: Jeroen Demeyer

[jdemeyer]

comment:10

Merged with 6.4.beta4 and added some fixes. Please review.

[sagetrac-git]

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

463eb63

Rendering canvas3d in all doctests is too slow

[sagetrac-git]

Changed commit from 49ce20b to 463eb63

[sagetrac-git]

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

711127f

Merge remote-tracking branch 'origin/develop' into ticket/16640

[sagetrac-git]

Changed commit from 463eb63 to 711127f

[kcrisman]

comment:13

Hi Jeroen: Your first two commits are fine, as is the first commit of Martin's. (Currently looking at his main, second one.) How much slower did the doctests become without your last commit? (Martin, if you see this, what was your thinking on changing the doctest mode?)

[gagern]

comment:14

Hi, sorry I didn't respond to the commits by Jeroen any sooner.

Replying to @kcrisman:

Hi Jeroen: Your first two commits are fine,

I agree. Thanks a lot!

(Martin, if you see this, what was your thinking on changing the doctest mode?)

My main reason for modifying that line at all was the fact that canvas3d only works in embedded mode, so having it available otherwise might confuse users. My reson for including it in doctests is simply because all the other possible viewers are active in doctest mode as well. There is a doctest specifically for the canvas3d case, but if I restrict canvas3d to embedded mode only, that one would fail. So staying in line with all the other tests seemed the reasonable thing to do.

If things really are too slow, I'd change that to
if viewer == 'canvas3d' and (DOCTEST_MODE or EMBEDDED_MODE):
so we can have that one explicit doctest nevertheless. But first I'd like to quantify that effect on test execution time, and right now I have a build problem.

[jdemeyer]

comment:15

Always doing canvas3d is really too slow, I remember one file(!) taking over 1000s to doctest.

[jdemeyer]

comment:53

Replying to @kcrisman:

despite what you just said earlier that making canvas3d guys was too long!

Okay, sorry, I see what you did now - got a little too excited there. What do you think about testing the warning?

I'm too confused, what are you (plural) trying to say here?

[kcrisman]

comment:54

Here is the short version:

p.show(viewer='canvas3d')

raises an error in the command line but not during doctesting because of the way you wrote the code (creates the file). Perhaps the warning should also be doctested.

[jdemeyer]

comment:55

Replying to @kcrisman:

Perhaps the warning should also be doctested.

That's possible but requires some messing around with DOCTEST_MODE. Given that it's a minor thing and that Volker has plans to change all this code anyway, I feel that it's not worth the trouble. But if you insist, I will add that test.

PS: I assume you mean "error", not "warning".

[kcrisman]

comment:56

Perhaps the warning should also be doctested.

That's possible but requires some messing around with DOCTEST_MODE.

Yes.

Given that it's a minor thing and that Volker has plans to change all this code anyway, I feel that it's not worth the trouble. But if you insist, I will add that test.

Ok.

PS: I assume you mean "error", not "warning".

Yes, though I recall writing both of those a few times and then accidentally closing the browser/getting timed out!

[vbraun]

comment:57

Conflicts with 6.4, please merge.

[sagetrac-git]

Changed commit from 3852a0a to e83f839

[sagetrac-git]

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

e83f839

Merge tag '6.4' into ticket/16640

[jdemeyer]

comment:59

Done, the conflict was the removal of a redundant import of tmp_filename.

[jdemeyer]

comment:60

sage -t --long src/sage/repl/display/formatter.py
**********************************************************************
File "src/sage/repl/display/formatter.py", line 320, in sage.repl.display.formatter.SageNBTextFormatter.__call__
Failed example:
    fmt(FooGraphics())
Expected:
    showing graphics
    ''
Got:
    showing graphics
    doctest:239: FormatterWarning: Exception in text/plain formatter: [Errno 2] No such file or directory: '/nonexistent.png'
**********************************************************************

[sagetrac-git]

Changed commit from e83f839 to 034fa58

[sagetrac-git]

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

034fa58

Fix doctest to use existing file

[vbraun]

Changed branch from u/jdemeyer/ticket/16640 to 034fa58