bugGNU Octave - Bugs: bug #62215, GUI tempfiles do not honor tmpdir...

 
 

bug #62215: GUI tempfiles do not honor tmpdir environment variable

Submitter:  Nicholas Jankowski <nrjank>
Submitted:  Thu 24 Mar 2022 12:25:49 PM UTC
   
 
Category:  None Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Other
Status:  Fixed Assigned to:  None
Originator Name:  Nicholas Jaknkowski Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 03 Apr 2022 12:36:16 PM UTC, comment #13: 

I have updated resource_manager::create_tmp_file() with cset http://hg.savannah.gnu.org/hgweb/octave/rev/b77b321f1ac5 such that the new function is used for the temp. directory.

Torsten Lilge <ttl>
Group Member
Sun 03 Apr 2022 10:43:00 AM UTC, comment #12: 

Thanks for checking!

I cleaned up the patch and pushed it here:
https://hg.savannah.gnu.org/hgweb/octave/rev/1921d9d0e62b

Markus Mützel <mmuetzel>
Group administrator
Sun 03 Apr 2022 07:55:47 AM UTC, comment #11: 

@Markus, thanks for the patch. Using the new function get_temp_directory() instead of the separate function in resource_manager works well, too (tested on Debian).

Torsten Lilge <ttl>
Group Member
Sat 02 Apr 2022 06:04:35 PM UTC, comment #10: 

I'm attaching a patch that implements what I described in comment #8.

Only tested on Ubuntu 21.10 so far. I'll try and test on Windows later.

(file #53036)

Markus Mützel <mmuetzel>
Group administrator
Sat 02 Apr 2022 05:24:31 PM UTC, comment #9: 

Yes, this sounds totally reasonable. Then I agree that the best option is to add a TMPDIR check to 'octave::sys::do_get_temp_directory'.

Torsten Lilge <ttl>
Group Member
Sat 02 Apr 2022 04:20:04 PM UTC, comment #8: 

That really is quite confusing:
We have `octave::env::get_temp_directory` that checks "TMP" but not "TMPDIR" (the canonical environment variable for the temporary directory on POSIX) before resorting to P_tmpdir (which is often just set to "/tmp" or "/var/tmp" in <stdio.h> on Linux).
Then, we have the Octave function `tempdir` that checks "TMPDIR" but not "TMP" (nor "TEMP" on Windows fwiw) before resorting to P_tmpdir.
Then, there is the Octave function `tempname` that manually checks "TMPDIR" before passing on to `octave::env::tempnam` that hands off to `octave::env::get_temp_directory`.
Then, there is the Octave function `tmpfile` that hands off to the gnulib function `tmpfile` which I haven't checked what it does in which order.
Now, there is also a new function in libgui that does something similar. But probably still different from any of the above.
And there are probably other functions that I have missed.

Maybe, it is time to consolidate all of this and merge them into one single function that results in the same temporary directory in all of these cases.

Personally, I wouldn't bother adding in Qt as another source for a temporary directory. If our "new and shiny" temp_directory function ends up with a directory that can't be used, we are probably in deep trouble anyway... (But that's just my 2 cents.)

Imho, we could probably add a check for "TMPDIR" as the first step in `octave::sys::do_get_temp_directory` (on all platforms). Other than that, I'd say it is the most complete one. The fallbacks on Windows and POSIX also look reasonable to me. I'd guess that Qt will probably not do anything much fancier either...

It also helps that `octave::env::get_temp_directory` is exported from liboctave. It should be usable in all other Octave modules.

Then, we could retire the Octave function `tempdir` and replace it with a DEFUN that just wraps around `octave::env::get_temp_directory`.
`tempname` already relies on the same function. It could probably drop the check for "TMPDIR" if we move that some levels deeper.
We could probably also unify the implementations of `tempname` and `tmpfile` and retire the gnulib wrapper...
After that, at least those functions should use a consistent temporary directory. And that updated function could probably also be used in libgui.

What do you think?

Markus Mützel <mmuetzel>
Group administrator
Sat 02 Apr 2022 06:51:03 AM UTC, comment #7: 

Ah, I wasn't aware of this function, thanks for pointing this out.

However, octave::env::get_temp_directory does not check for tmpdir, which is explicitly requested in this bug report and the fallback is hard coded, e.g., c:\temp for windows, whereas I would like to use the standard location that Qt provides as fallback if all checked environment variables aren't set.

Torsten Lilge <ttl>
Group Member
Fri 01 Apr 2022 11:10:15 AM UTC, comment #6: 

Instead of re-implementing the same thing in various places, could we just use `octave::env::get_temp_directory`` from `oct-env.h` (liboctave) everywhere?

Markus Mützel <mmuetzel>
Group administrator
Thu 31 Mar 2022 01:46:54 PM UTC, comment #5: 

I have pushed cset http://hg.savannah.gnu.org/hgweb/octave/rev/2bb539746697 which fixes the issue with the ignored user settings for the temp directory.

Torsten Lilge <ttl>
Group Member
Sat 26 Mar 2022 03:30:32 PM UTC, comment #4: 

Okay. then the most general approach would a function like the get_temp_dir in mkoctfile.cc.in but with QStandardPaths::writableLocation (QStandardPaths::TempLocation) as fallback.

I have updated the title in order to reflect the fact that the temporary files used by the GUI are all affected, regardless the debug state.


Torsten Lilge <ttl>
Group Member
Thu 24 Mar 2022 07:11:05 PM UTC, comment #3: 

There are so many ways to create temp files and temp file names and each seems to have a different way of managing (or not) security and things like environment variables or the setting of P_tmpdir, etc.  I agree that it would be good to have consistent behavior, I'm just not sure how to get there.

John W. Eaton <jwe>
Group administrator
Thu 24 Mar 2022 06:34:01 PM UTC, comment #2: 

ok.  the issues that brought bug #55814 to light was that certain platforms had issues performing certain activities in the default windows temp location (mainly compiling pkgs and other executable-like activity) so not respecting the octave tempdir was a problem.  I'm not certain whether cases exist where F9 executed code could cause the same issue, but I would think it would be preferable to follow that setting if it is not overly burdensome to do so.

Nicholas Jankowski <nrjank>
Group Member
Thu 24 Mar 2022 06:26:18 PM UTC, comment #1: 

The temp files for the F9 mechanism are created by QDir::temp() and QDir::tempPath () which return the system's temp directory. My intention was that the Qt framework might provide a robust cross-platform behavior related to temporary files.

I do not think that the debug mode has an influence on the temporary directory. It is more probable that the way how the interpreter's error message is patched for the case of selected code execution (remove line number and filename) does not work in the debug mode as expected.


Torsten Lilge <ttl>
Group Member
Thu 24 Mar 2022 12:25:49 PM UTC, original submission:  

first identified in bug #62105 when using the editor in debug mode. if  using F9 to execute a code block an error is indicated pointing to the temporary file created for executing the code.  However, despite tmpdir being set, the temp file appears to be located in the system TEMP location. Unsure if this is an issue with the GUI or with the debug mode, and if it affects more than windows (verified on 6.4.0 on Windows 10).  This seems similiar to bug #55814, where mkoctfile was ignoring tmpdir, but that was fixed a few versions ago.

to reproduce:


*create c:\octave\temp*

>> getenv('temp')
ans = C:\Users\<USERNAME_in8.3format>\AppData\Local\Temp

>> getenv('tmp')
ans = C:\Users\<USERNAME_in8.3format>\AppData\Local\Temp

>> setenv('tmpdir', 'c:\octave\temp')

>> tempdir
ans = c:\Octave\Temp\

>> edit mean

*set breakpoint at first line of code - line 94*

>> mean(1)
stopped in mean at line 94 [C:\Octave\octave-6.4.0-w64\
mingw64\share\octave\6.4.0\m\statistics\mean.m]
94:   if (nargin < 1 || nargin > 4)

*highilght uninitialized variable 'nd' on line 101, press F9*

debug> nd
error: 'nd' undefined near line 1, column 1
error: called from
    C:/Users/<USERNAME>/AppData/Local/Temp/octave/octave
_klgahA.m at line 1 column 1

debug> tempdir
ans = c:\octave\temp


(not sure what to tag category and item group as. feel free to correct.)

Nicholas Jankowski <nrjank>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #53036:  bug62215-tmp.patch added by mmuetzel (7KiB - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by ttl (Posted a comment)
  • -email is unavailable- added by nrjank (Submitted the item)
  • -email is unavailable- added by nrjank
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-04-02 mmuetzel Attached File- Added bug62215-tmp.patch, #53036
    2022-03-31 ttl StatusNone Fixed
        Open/ClosedOpen Closed
        Release6.4.0 dev
        Operating SystemMicrosoft Windows Any
    2022-03-26 ttl SummaryGUI / debug mode does not honor tmpdir environment variable GUI tempfiles do not honor tmpdir environment variable
    2022-03-24 nrjank Carbon-Copy- Added ttl

    Back to the top

    Powered by Savane 3.13-4b48.
    Corresponding source code