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)

Submitted by:  Avinoam Kalma <avinoam>
Submitted on:  Tue 28 Jun 2016 04:56:51 AM UTC  
 
Category: Octave FunctionSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Incorrect Result
Status: FixedAssigned to: Mike Miller <mtmiller>
Originator Name: Open/Closed: Closed
Release: devOperating System: Microsoft Windows

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Wed 13 Jul 2016 12:02:50 AM UTC, comment #21:

Pushed to default

http://hg.savannah.gnu.org/hgweb/octave/rev/67a5341edf15

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
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 AdministratorIn charge of this item.
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 AdministratorIn charge of this item.
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 AdministratorIn charge of this item.
Mon 11 Jul 2016 06:36:01 PM UTC, comment #15:

Indeed, freport gives:

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 AdministratorIn charge of this item.
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

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 AdministratorIn charge of this item.
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

... into:

... 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)

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

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:

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:

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):

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 AdministratorIn charge of this item.
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

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:

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

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

(file #37779)

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
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 AdministratorIn charge of this item.
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 AdministratorIn charge of this item.
Wed 29 Jun 2016 08:27:21 PM UTC, comment #2:

In the command

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

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?

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Tue 28 Jun 2016 04:56:51 AM UTC, original submission:

test printd is

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

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 File(s):
   
   
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
  • -unavailable- added by lachlan (Updated the item)
  • -unavailable- added by philipnienhuis
  • -unavailable- added by mtmiller (Posted a comment)
  • -unavailable- added by avinoam (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 16 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 13 Jul 2016 12:02:50 AM UTCmtmillerStatusPatch Submitted=>Fixed
      Open/ClosedOpen=>Closed
    Tue 12 Jul 2016 10:47:09 PM UTClachlanAssigned toNone=>mtmiller
    Mon 11 Jul 2016 09:33:17 PM UTCmtmillerAttached File-=>Added bug-48326-binary.diff, #37815
    Mon 11 Jul 2016 06:36:28 PM UTCphilipnienhuisCarbon-Copy-=>Added philipnienhuis
    Mon 11 Jul 2016 06:25:48 PM UTCmtmillerAttached File-=>Added bug-48326.patch, #37812
    Mon 11 Jul 2016 05:58:30 PM UTCphilipnienhuisAttached File-=>Added bug48326_printd.cset, #37809
      StatusIn Progress=>Patch Submitted
    Mon 11 Jul 2016 05:00:14 PM UTCmtmillerSummarytest printd fails in Win-7=>mkstemp opens file in text mode on Windows (test printd fails)
    Mon 11 Jul 2016 10:33:56 AM UTCphilipnienhuisStatusNeed Info=>In Progress
    Fri 08 Jul 2016 11:44:49 PM UTCmtmillerAttached File-=>Added bug-48326-debug.patch, #37779
    Thu 30 Jun 2016 08:08:49 PM UTCmtmillerStatusFixed=>Need Info
      Open/ClosedClosed=>Open
    Thu 30 Jun 2016 05:38:27 PM UTCmtmillerStatusNeed Info=>Fixed
      Open/ClosedOpen=>Closed
    Wed 29 Jun 2016 07:47:33 PM UTCmtmillerStatusNone=>Need Info

    Back to the top


    Powered by Savane 3.1-cleanup1