bugGNU Octave - Bugs: bug #48326, mkstemp opens file in text mode on...

 
 

bug #48326: mkstemp opens file in text mode on Windows (test printd fails)

Submitter:  Avinoam Kalma <avinoam>
Submitted:  Tue 28 Jun 2016 04:56:51 AM UTC
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  mtmiller
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Microsoft Windows
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment (Rich Markup)
   

Comment Type & Canned Response:
       

 

( Jump to the original submission )

Wed 13 Jul 2016 12:02:50 AM UTC, comment #21: 
Mike Miller <mtmiller>
Project Administrator
Tue 12 Jul 2016 10:14:48 PM UTC, comment #20: 

Great, thank you for confirming, I'll push this fix shortly.

Sure, it's not an atomic operation from a processor instruction point of view. But it opens the file in a safe exclusive manner before it returns the file name to you. So not only do you get a temporary file name, but you get an open file that is guaranteed to have been created by your process with permissions to only be readable and writable by your process.

There is no way to tell from your example whether the file name returned by tempname() existed before I opened it, if it's empty or not, if I even own the file or if someone else owns it, or what its user access permissions are set to.

Anyway, glad to see we can get this working.

Mike Miller <mtmiller>
Project Administrator
Tue 12 Jul 2016 09:44:52 PM UTC, comment #19: 

Mike, with the patch,"test printd" PASSes 1 of 1 tests. Not bad for a one-liner :-)

BTW what you call an "atomic operation" in comment #16 is rather an "atomic function". Strictly speaking then I'd call mkstemp less unsafe than e.g.,:

fid = fopen (tempname (), "w+b");
fname = fopen (fid);

Philip Nienhuis <philipnienhuis>
Project Member
Mon 11 Jul 2016 10:03:57 PM UTC, comment #18: 

As to the last sentence in comment #15: of course testing is always OK; it's just that in C++ land I'm a bit clueless.

I just finished a cross-build and the new patch came too late. Tomorrow there's another chance :-)

Thanks for the explanation - an atomic operation is a good motive.

Philip Nienhuis <philipnienhuis>
Project Member
Mon 11 Jul 2016 09:33:17 PM UTC, comment #17: 

Please try rebuilding the default branch with the attached patch. I think the thing that was missing here was the Windows definition of the O_BINARY file option, so it was defaulting to text.

The fdopen function takes the stream options to set whether binary mode is on or off, but appears to have no effect, what matters is how the low-level file descriptor was opened.

(file #37815)

Mike Miller <mtmiller>
Project Administrator
Mon 11 Jul 2016 07:13:48 PM UTC, comment #16: 

Thanks for all the testing. We'll still need someone willing to help test or debug mkstemp or fdopen on Windows.

The reason mkstemp is safer is because it opens a temporary file for you and gives you the name of the file in one atomic operation. If you call a function like tempname() to just get a string, and then subsequently open the file, there is the possibility that someone has intercepted that file name and opened it before you.

Of course if you always create temporary files in a per-user unique temporary directory this is slightly mitigated, although still at risk to those with root level permissions on the system.

Mike Miller <mtmiller>
Project Administrator
Mon 11 Jul 2016 06:36:01 PM UTC, comment #15: 

Indeed, freport gives:

>> test printd
stopped in C:\Programs\Octave\Octave-4.1.0+VEp_w64_20160708\share\octave\4.1.0+\m\plot
\util\printd.m at line 47
47:   [pf, tempf, mag] = mkstemp ("oct-XXXXXX", 1);
debug>
:
debug> freport (pf)
warning: freport: ignoring extra arguments
warning: called from
    printd at line 48 column 1
    __test__ at line 11 column 2
    test at line 583 column 13

  number  mode  arch       name
  ------  ----  ----       ----
     0     r    ieee-le    stdin
     1     w    ieee-le    stdout
     2     w    ieee-le    stderr
     3     w+b  ieee-le    oct-GCvhOg

Didn't know that tempname() is less safe (why is that?)
I agree fixing mkstemp is superior. I'll leave here - as far as this bug goes I've done what I could do :-)

Philip Nienhuis <philipnienhuis>
Project Member
Mon 11 Jul 2016 06:25:48 PM UTC, comment #14: 

I don't see a point in patching the failing test until we can fix mkstemp. And I don't think we want to encourage changes that use the less safe tempname function when mkstemp should be preferred (when Matlab compatibility is not an issue).

If anything, I think a better workaround would be to replace all carriage returns in the r4 character array (I apparently wrote this a while ago but hasn't been pushed since we are still debugging here).

(file #37812)

Mike Miller <mtmiller>
Project Administrator
Mon 11 Jul 2016 05:58:30 PM UTC, comment #13: 

Here's cset for printd (replacing mkstemp() with fopen().
It'll mend the immediate problem with printd.

(file #37809)

Philip Nienhuis <philipnienhuis>
Project Member
Mon 11 Jul 2016 05:00:14 PM UTC, comment #12: 

Yes, I think that was evident from

debug: supposedly writing to output file 217 bytes
numbytes =  217
debug: after writing to temp file oct-FJAr0M, size = 224 bytes

Retitled to problem that needs fixing.

If you call "freport" after opening a file with mkstemp, it shows it listed as binary mode ("w+b"), right? So the error is probably lower in the libraries, maybe the fdopen() function on Windows is failing to set the open mode to binary but not throwing an error.

Mike Miller <mtmiller>
Project Administrator
Mon 11 Jul 2016 10:33:56 AM UTC, comment #11: 

@Mike:
I think I nailed it down. It is mkstemp() that does not open a temp file in binary mode as it should.

Indeed, after changing L.47 in printd.m

  [pf, tempf, mag] = mkstemp ("oct-XXXXXX", 1);

... into:

  tempf = tempname (".");
  pf = fopen (tempf, "w+b");

... printd.m PASSes its sole test.

(I'll save you the exercitions with fread before/after fclose/fopen, I think caching and file buffers conceal what is going on)

I can't make a cset for this change right now, hopefully tonight I'll have time. But as it is actually a bug in mkstemp, do we need a cset for printd.m at all?
A cset a la above would make printd.m more robust.

A fix for mkstemp() is needed in any case. I can't as it's C++ stuff.

Philip Nienhuis <philipnienhuis>
Project Member
Sun 10 Jul 2016 09:44:18 PM UTC, comment #10: 

(Edited by hand)

>> edit printd
>> test printd
debug: supposedly writing to output file 217 bytes
numbytes =  217
debug: after writing to temp file oct-FJAr0M, size = 224 bytes
debug: after copying to dest file test_p.txt, size = 224 bytes
debug: after closing temp file oct-FJAr0M, size = 224 bytes
debug: just before returning, output file test_p.txt, size = 224 bytes
***** test
:
<snip>
:
ASSERT errors for:  assert (r4,r2)
:
<snip>
:
   Strings don't match
>>

Again I tried the debug option at L.47 (.....mkstemp....)

philip@DeskPRN /c/tmp
### Just after the fprintf line: file seems empty
$ ls -l oct-*
-rw-r--r-- 1 philip Administrators 0 Jul 10 23:13 oct-X5WOJL

philip@DeskPRN /c/tmp
$ cat oct-X5WOJL

### Indeed, empty. Then, after the fclose() stmt:
philip@DeskPRN /c/tmp
$ ls -l oct-*
-rw-r--r-- 1 philip Administrators 224 Jul 10 23:14 oct-X5WOJL

philip@DeskPRN /c/tmp

Apparently there's write caching going on.
As the C: drive is on an SSD, I repeated it on drive X: which is on a HDD with rotating platters; but I got the same results.

But I think something else is going on here.
I removed your patch from comment #9 and inserted an fread() stmt after frewind:
tstscr = fread (pf, Inf,'char=>char")';
numel (tstscr):

That gave 217 bytes, so my conclusion is that file (pf) (made by mkstemp) is written fine.

Then I added similar fread/numel statements after the copy in the "txt" switch clause, after the "system (enscr)" statement:

frewind (pf)
tstscr = fread (pf, Inf,'char=>char")';
numel (tstscr)
fid = fopen (filename);
tstscr2 = fread (fid,  Inf,'char=>char")';
numel (tstscr2)

and the answers I got:
217
224

so it appears to be the MSYS "cp" command that adds the CR characters.

But in a separate MSYS shell:

$ ls -l
total 990735
:
-rw-r--r-- 1 philip Administrators       224 Jul 10 23:27 oct-jfiupM
-rw-r--r-- 1 philip Administrators       224 Jul 10 23:30 test_p.txt
:

so MSYS thinks both files are 224 bytes, in spite of Octave's fread() results mentioned earlier.

Really enigmatic.

(sorry bed time, work week starts tomorrow)

Philip Nienhuis <philipnienhuis>
Project Member
Sat 09 Jul 2016 11:49:31 PM UTC, comment #9: 

Ok, can you try moving the fclose of the temporary file and printing the file size after it's closed? Something like this change (with a pause inserted as well):

diff --git a/scripts/plot/util/printd.m b/scripts/plot/util/printd.m
--- a/scripts/plot/util/printd.m
+++ b/scripts/plot/util/printd.m
@@ -46,7 +46,10 @@ function pr_out = printd (obj, filename)
   opt = substr (filename, sufix+1);
   [pf, tempf, mag] = mkstemp ("oct-XXXXXX", 1);
   fprintf (pf, "%s", disp (obj));
-  frewind (pf);
+  fclose (pf);
+  pause (2);
+  tmp = dir (tempf);
+  fprintf (stdout, "size of temp file on filesystem = %ld bytes\n", tmp.bytes);

   ## It seems best to only use convert for image output.  Its ps and pdf
   ## are badly rendered.
@@ -77,7 +80,6 @@ function pr_out = printd (obj, filename)
       error ("printd: unknown conversion type: %s.\nOptions are: pdf ps eps txt jpg jpeg", opt);

   endswitch
-  fclose (pf);
   delete (tempf);
   pr_out = sprintf ("%s file %s written\n", opt, filename);

We need to know if it's the writing of the file by Octave, or if it's the cp command executed by the shell that converts the line endings. Or does moving the fclose() up higher in the function and adding a pause() make the test start passing?

Mike Miller <mtmiller>
Project Administrator
Sat 09 Jul 2016 08:18:57 PM UTC, comment #8: 

Playing a bit with mkstemp and fprintf doesn't give a clue.
It all works as advertised, unlike what the result in my comment #6 seems to suggest.  So I take back the last sentence from that comment - although the MSYS shell results remains.

Philip Nienhuis <philipnienhuis>
Project Member
Sat 09 Jul 2016 02:55:06 PM UTC, comment #7: 

I get the same results on 32-bit version

>> test printd
debug: supposedly writing to output file 217 bytes
debug: after writing to temp file oct-Syrg09, size = 0 bytes
debug: after copying to dest file test_p.txt, size = 224 bytes
debug: after closing temp file oct-Syrg09, size = 224 bytes
debug: just before returning, output file test_p.txt, size = 224 bytes
***** test
 r2 = char (
 "stem step: 10, data: unsorted.",
 "Hinges:    lo: 12, hi: 42"     ,
 "   1 | 22118"                  ,
 "   2 | 28"                     ,
 "   3 | 98"                     ,
 "   4 | 244"                    ,
 "   5 | 2"                      );
 printd (r2, "test_p.txt");
 r4 = fileread ("test_p.txt");
 delete ("test_p.txt");
 r2 = disp (r2);
 assert (r4, r2);
!!!!! test failed
ASSERT errors for:  assert (r4,r2)

  Location  |  Observed  |  Expected  |  Reason
     []      stem step: 10, data: unsorted.
Hinges:    lo: 12, hi: 42
   1 | 22118
   2 | 28
   3 | 98
   4 | 244
   5 | 2
 stem step: 10, data: unsorted.
Hinges:    lo: 12, hi: 42
   1 | 22118
   2 | 28
   3 | 98
   4 | 244
   5 | 2
   Strings don't match

Avinoam Kalma <avinoam>
Project Member
Sat 09 Jul 2016 09:25:16 AM UTC, comment #6: 

After hand-editing printd.m and also changing L.49:
fprintf (pf, "%s", disp (obj));
into
numbytes = fprintf (pf, "%s", disp (obj))
I get:

>> test printd
debug: supposedly writing to output file 217 bytes
numbytes =  217
debug: after writing to temp file oct-wd3O07, size = 0 bytes
debug: after copying to dest file test_p.txt, size = 224 bytes
debug: after closing temp file oct-wd3O07, size = 224 bytes
debug: just before returning, output file test_p.txt, size = 224 bytes
***** test
 r2 = char (
 "stem step: 10, data: unsorted.",
 "Hinges:    lo: 12, hi: 42"     ,
 "   1 | 22118"                  ,
 "   2 | 28"                     ,
 "   3 | 98"                     ,
 "   4 | 244"                    ,
 "   5 | 2"                      );
 printd (r2, "test_p.txt");
 r4 = fileread ("test_p.txt");
 delete ("test_p.txt");
 r2 = disp (r2);
 assert (r4, r2);
!!!!! test failed
ASSERT errors for:  assert (r4,r2)

  Location  |  Observed  |  Expected  |  Reason
     []      stem step: 10, data: unsorted.
Hinges:    lo: 12, hi: 42
   1 | 22118
   2 | 28
   3 | 98
   4 | 244
   5 | 2
 stem step: 10, data: unsorted.
Hinges:    lo: 12, hi: 42
   1 | 22118
   2 | 28
   3 | 98
   4 | 244
   5 | 2
   Strings don't match
>>

Introducing a debug stop on L. 49, after the fprintf statement I went into an MSYS shell and did:

$ ls -l oct-x*
-rw-r--r-- 1 philip Administrators 224 Jul  9 11:19 oct-xrikuy

so it seems fprintf introduces the Windows style EOLs (in spite of implicitly reporting it doesn't).

Philip Nienhuis <philipnienhuis>
Project Member
Fri 08 Jul 2016 11:44:49 PM UTC, comment #5: 

Avinoam, can you (or anyone else paying attention) apply the attached patch and report back the debug output from "test printd"?

I see the following on Debian

>> test printd
debug: supposedly writing to output file 217 bytes
debug: after writing to temp file oct-ro9BSj, size = 217 bytes
debug: after copying to dest file test_p.txt, size = 217 bytes
debug: after closing temp file oct-ro9BSj, size = 217 bytes
debug: just before returning, output file test_p.txt, size = 217 bytes
PASSES 1 out of 1 test

(file #37779)

Mike Miller <mtmiller>
Project Administrator
Thu 30 Jun 2016 08:08:49 PM UTC, comment #4: 

Reopening because that was the wrong fix.

The fileread function does read a file in binary mode, so you will get carriage return characters in the return value if the file was written with Windows line endings.

There may be a problem somewhere else in either the cp utility, or the mkstemp function, or the fprintf function.

Mike Miller <mtmiller>
Project Administrator
Thu 30 Jun 2016 05:38:27 PM UTC, comment #3: 

Fixed on default. Thank you for the testing and helping identify the problem!

http://hg.savannah.gnu.org/hgweb/octave/rev/f078dd2c6f27

Mike Miller <mtmiller>
Project Administrator
Wed 29 Jun 2016 08:27:21 PM UTC, comment #2: 

In the command

printd (r2, "test_p.txt");

the file test_p.txt has CR+LF (Windows end-of-line), and not
only LF, as it was in Octave 4.0.2

The fix

fid = fopen (filename, "rt");

indeed solves this, and the test pass.

Avinoam Kalma <avinoam>
Project Member
Wed 29 Jun 2016 07:47:33 PM UTC, comment #1: 

So either printd or fileread or both are at fault here. The test as it is written looks correct. My guess would be that printd is writing a valid text file with Windows line endings, and fileread is not reading it back correctly.

Can you check whether the file created by printd has Unix or Windows line endings?

Can you try this simple patch to fileread.m and see if it fixes this?

diff --git a/scripts/io/fileread.m b/scripts/io/fileread.m
--- a/scripts/io/fileread.m
+++ b/scripts/io/fileread.m
@@ -32,7 +32,7 @@ function str = fileread (filename)
     error ("fileread: FILENAME argument must be a string");
   endif

-  fid = fopen (filename, "r");
+  fid = fopen (filename, "rt");
   if (fid < 0)
     error ("fileread: cannot open file");
   endif

Mike Miller <mtmiller>
Project Administrator
Tue 28 Jun 2016 04:56:51 AM UTC, original submission:  

test printd is

r2 = char (
"stem step: 10, data: unsorted.",
"Hinges:    lo: 12, hi: 42"     ,
"   1 | 22118"                  ,
"   2 | 28"                     ,
"   3 | 98"                     ,
"   4 | 244"                    ,
"   5 | 2"                      );
printd (r2, "test_p.txt");
r4 = fileread ("test_p.txt");
delete ("test_p.txt");
r2 = disp (r2);
assert (r4, r2);

 

On linux the test pass. On Win-7 it fails.

whos
...
       r2          1x217                      217  char
       r4          1x224                      224  char

The problem is with the "printd" command, which adds 7 CR
characters (one for each line), so the final error is
"Strings don't match"

Avinoam Kalma <avinoam>
Project Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #37815:  bug-48326-binary.diff added by mtmiller (345B - text/x-diff)
file #37812:  bug-48326.patch added by mtmiller (654B - text/x-diff)
file #37809:  bug48326_printd.cset added by philipnienhuis (821B - application/octet-stream)
file #37779:  bug-48326-debug.patch added by mtmiller (1KiB - text/x-diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by lachlan (Updated the item)
  • -email is unavailable- added by philipnienhuis
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by avinoam (Submitted the item)
  •  

    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 project members can vote.

     

     

     

     

    Follow 16 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-07-13 mtmiller StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2016-07-12 lachlan Assigned toNone mtmiller
    2016-07-11 mtmiller Attached File- Added bug-48326-binary.diff, #37815
    2016-07-11 philipnienhuis Carbon-Copy- Added philipnienhuis
    2016-07-11 mtmiller Attached File- Added bug-48326.patch, #37812
    2016-07-11 philipnienhuis Attached File- Added bug48326_printd.cset, #37809
        StatusIn Progress Patch Submitted
    2016-07-11 mtmiller Summarytest printd fails in Win-7 mkstemp opens file in text mode on Windows (test printd fails)
    2016-07-11 philipnienhuis StatusNeed Info In Progress
    2016-07-08 mtmiller Attached File- Added bug-48326-debug.patch, #37779
    2016-06-30 mtmiller StatusFixed Need Info
        Open/ClosedClosed Open
    2016-06-30 mtmiller StatusNeed Info Fixed
        Open/ClosedOpen Closed
    2016-06-29 mtmiller StatusNone Need Info

    Back to the top

    Powered by Savane 3.9