bugGNU Octave - Bugs: bug #52116, Textscan filepointer bug is still...

 
 

bug #52116: Textscan filepointer bug is still present in Windows build

Submitter:  None
Submitted:  Tue 26 Sep 2017 07:05:24 AM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Cees Meijer Originator Email:  -email is unavailable-
Open/Closed:  * Closed Release:  * 4.2.1
Operating System:  * Microsoft Windows Fixed Release:  None
Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 26 Oct 2017 04:15:16 AM UTC, comment #18: 

Wow, I'm not sure I've seen a single one-line patch wipe out 3 different bugs.  Well done Dan!

I committed the patch on the default branch here (http://hg.savannah.gnu.org/hgweb/octave/rev/cc631e9926e1).  In the comment message I chose to list each bug fixed.  This makes it possible to grep through the Mercurial log to find out which patch fixed a particular bug.

I also grafted the changeset on to the stable branch.  I'm not sure there will be a 4.2.2 release, but if there is then it will be a part of it.

Finally, the code reuse patch applied cleanly and "make check" passed so I also committed that here (http://hg.savannah.gnu.org/hgweb/octave/rev/89167cc74260).

Closing this and all connected reports.

Rik <rik5>
Group administrator
Tue 24 Oct 2017 07:19:39 PM UTC, comment #17: 

Dan, your patches also seem to have fixed bug #51707.
Good work!

Philip Nienhuis <philipnienhuis>
Group Member
Sun 22 Oct 2017 07:46:53 PM UTC, comment #16: 

I've applied both patches and I can conform the issue in this bug report is fixed.
The issue in bug #50619 is also fixed.

However the issue in bug #41579 (ftell offset one less relative to Matlab's offset) is still there.

Philip Nienhuis <philipnienhuis>
Group Member
Sun 22 Oct 2017 07:08:03 PM UTC, comment #15: 

Sorry, the first line in the code of my test case got corrupted. Here it is again:


data1 = "aaaaaaaaaaaabbbbccccccccccccccccccdddeeefffggggghhhhiiiiijjjjjkkklllllllllmmmmmmmmn\n";
data2 = "asdf\n";
data3 = "a,bbbb,cccc,ddddddd,eeeeeeeeeeeee,fffff,ggggggggg,hhhhhhhhh,xxxxxxxxx\n";
data4 = "1,2,3,4,5,6,7,8,9\n";

filename = tempname ();
fid = fopen (filename, "w");
fputs (fid, data1);
fputs (fid, data2);
fputs (fid, data3);
fputs (fid, data4);
fclose (fid);

fid = fopen (filename);
header = textscan (fid, "%s", 2, "delimiter", "");
result = textscan (fid, "%s%s", 1, "delimiter", ",");
fclose (fid);
delete (filename);

assert (result{1,2}, {"bbbb"});


(Note: This bug happens under Linux as well as Windows.)

Hartmut <hardy>
Sun 22 Oct 2017 07:00:58 PM UTC, comment #14: 

I have also hit this bug. (My application case worked fine with previous versions of Octave, version 4.0.3 for example. So this might be a regression bug.)

I have compiled a current code version of Octave (hg id 209e4f34d243) with the first patch from comment #12, i.e. file #41900. And this patch FIXED the issue for me. Running "make test" also revealed no other failing tests.

I also tried to construct a minimal example to be used as a new test case for the textscan function. Here it is:


data2 = "asdf\n";
data3 = "a,bbbb,cccc,ddddddd,eeeeeeeeeeeee,fffff,ggggggggg,hhhhhhhhh,xxxxxxxxx\n";
data4 = "1,2,3,4,5,6,7,8,9\n";

filename = tempname ();
fid = fopen (filename, "w");
fputs (fid, data1);
fputs (fid, data2);
fputs (fid, data3);
fputs (fid, data4);
fclose (fid);

fid = fopen (filename);
header = textscan (fid, "%s", 2, "delimiter", "");
result = textscan (fid, "%s%s", 1, "delimiter", ",");
fclose (fid);
delete (filename);

assert (result{1,2}, {"bbbb"});


This test fails with Octave 4.2.1, but succeeds with dev Octave plus the mentioned patch. (But it also fails with Octave 4.0.3...)

Would it be possible to include this littel patch (at least file #41900) into the stable branch and not only into the default branch?

(I have not tested the second patch, file #41901 .)

Hartmut <hardy>
Sat 21 Oct 2017 09:05:24 PM UTC, comment #13: 

@jwe: Can you review Dan's patch for this report from about a month ago?

Also, added a dependency for bug #50619 on this report which can also be closed after this patch is finished.

Rik <rik5>
Group administrator
Wed 27 Sep 2017 04:10:00 PM UTC, comment #12: 

Yes, I forgot; patch attached.

I'm also attaching a non-bug patch to reuse some code of one delimited_stream::delimited_stream constructor from the other and remove a FIXME.  (I think that's what that FIXME is about.)  However, I don't know what script to use to test that it works.



(file #41900, file #41901)

Dan Sebald <sebald>
Wed 27 Sep 2017 10:28:44 AM UTC, comment #11: 

Dan: I don't see a patch attached to this report.  Did you forget to attach it before submitting comment #10?

John W. Eaton <jwe>
Group administrator
Wed 27 Sep 2017 09:52:21 AM UTC, comment #10: 

I may have found the bug, please test the attached patch.

This very subtle change takes some thought.  As I mentioned, this delimited stream has to "put back" data into the conventional input stream, i.e., readjust the pointer, when it is done and there is unused data in its local buffer.  Currently that pointer is recorded from the input stream every time some data is read from the input stream.  But that is not correct because the input stream position is far ahead of the current delimited buffer's pointer, on average.

As an example, let's say the BUF_SIZE is N characters, something much larger than the data expected to be read, e.g. 1024.  buf_in_file starts at 0, then we read 8 bytes, etc.  The sequence of events would be


                   buf_in_file    iostream pos
initialize              0              0
refresh_buf             0              N
proc 8 bytes            8              N
refresh_buf             N              N+8
read 6 bytes            N+6            N+8
refresh_buf             N+8            N+8+6
read 9 bytes            N+8+9          N+8+6
refresh_buf             N+8+6          N+8+6+9
read 3 bytes            N+8+6+3        N+8+6+9
refresh_buf             N+8+6+9        N+8+6+9+3


At this point, we restore the stream position after having read only 26 bytes.  But the buf_in_file has a value of N+8+6+9, a value much greater than 26.  So that is where the error is.

Note that if we pick N larger than the amount of data in the file, we luck out because the input stream is only read the first time and successive times eof() tests true, i.e.,


                   buf_in_file    iostream pos
initialize              0              0
refresh_buf             0              filelen < N
proc 8 bytes            8              filelen
refresh_buf             8              filelen
read 6 bytes            8+6            filelen
refresh_buf             8+6            filelen
read 9 bytes            8+6+9          filelen
refresh_buf             8+6+9          filelen
read 3 bytes            8+6+9+3        filelen
refresh_buf             8+6+9+3        filelen


So, the change should fix the bug.  But there are probably still other bugs.  One thing is that because of the buffering of data there is an inherent assumption that a float value in ASCII form will be less than 80 characters, but someone could have test representations of numbers with more characters even though they wouldn't fit into a double float.  In order to handle such a thing in buffered data, the low-level deciphering of floats needs to be done at the same level.  That is we can't run scanf on something like


[123.456789101112] [41516171819 321.4567545667567...


where that first portion is length N=16 buffer and there is still a portion of ascii characters to come in from the stream.  I'm not going to concern myself with that though.  This delimited stream is much too complex to experiment with.

Dan Sebald <sebald>
Tue 26 Sep 2017 10:09:06 PM UTC, comment #9: 

I'm just adding a note here to diagnose the code.

I've printed out buf_size:

octave:4> A = textscan(fid,'%f %f %f %f %f %f %f %f %f %f %f %f %f',4,"endofline","\r\n");
BUF_SIZE=320

By my estimate there are 836 non-EOL characters in the data file.  The file size is 855 characters.  There look to be 10 lines in the file from an editor.  That works out to two characters for EOL; seeing this is windows, no surprise.  So starting from the end of line four in the data file, look 320 characters out and that puts us at


  1.81    101.73    234.13  -82.4  -59.6   -0.7   11.8  192.0  192.0  184.0  196.0  100 2147483647
  2.01     99.88    232.86  -79.6  -60.3    3.6   -8.0  191.0  190.0  180.0  195.0  100 2147483647
                    ^


That's not 99.88, the first read value of lastline=fgetl(fid), but it is close.  I'm probably not accounting for something, but the point is that the lost data has to do with the buffer size.

So, the obvious thought-o here is that specifying the number of lines to read, "4", means that the size of data is limited to 4*80, or some other value other than 80, without loss of generality.  That obviously isn't true.

Dan Sebald <sebald>
Tue 26 Sep 2017 06:50:15 PM UTC, comment #8: 

Thanks C.M.  I've run the script on Linux and verified the bug--the lastline variable beginning with 99.88.  So, this is not Windows OS specific.

This looks like a duplicate bug.  Replacing the textscan() line with


A = textscan(fid,'%f %f %f %f %f %f %f %f %f %f %f %f %f',4,"endofline","\r\n","BufSize",4096);


gives me the correct result.  See bug report

https://savannah.gnu.org/bugs/?50619#comment13

This textscan() looks like a high priority bug.  It needs a pretty significant restructuring because of the way it descends into a subroutine that reads one line at a time and tries to put stuff back into the buffer.  Information about what has been read is lost.

Dan Sebald <sebald>
Tue 26 Sep 2017 06:10:24 PM UTC, comment #7: 

I think I succeeded in including the files this time. But if not, just let me know and I'll try again.

Cees Meijer <cees_meijer>
Tue 26 Sep 2017 04:12:33 PM UTC, comment #6: 

you are right - I didn't look at the year, just month/day

John Donoghue <lostbard>
Group Member
Tue 26 Sep 2017 03:32:53 PM UTC, comment #5: 

John - I don't think that's right, bug #47671 was fixed before the 4.2.0 release, so it should have been fixed in both 4.2.0 and 4.2.1. But there are definitely other open bugs with textscan, including some that are Windows specific.

Cees - please provide an example of a file and a textscan call and show the problem you are having. You said you added something in comment #2, but I don't see any attachments here.

Mike Miller <mtmiller>
Group Member
Tue 26 Sep 2017 02:42:44 PM UTC, comment #4: 

Yes, I just followed the download link on the home page, and this leads to the ftp. So that could be the cause. But are there newer binaries available then ?

Cees Meijer <cees_meijer>
Tue 26 Sep 2017 12:06:23 PM UTC, comment #3: 

Are you using the binaries from https://ftp.gnu.org/gnu/octave/windows/ ?


If so, they were created before Bug # 47671 was fixed

John Donoghue <lostbard>
Group Member
Tue 26 Sep 2017 10:34:39 AM UTC, comment #2: 

I've added a sample file that comes close to the data I'm using, and an extract from my program that shows the problem.
The 'read_file' function should return the first line in the file, a  matrix with all values from the next 4 lines, and finally the sixth line in 'lastline'. And there it fails, because lastline will actually start reading from somewhere halfway line 8.

Cees Meijer <cees_meijer>
Tue 26 Sep 2017 08:06:38 AM UTC, comment #1: 

One of these bug reports might be relevant:

https://savannah.gnu.org/bugs/?50485
https://savannah.gnu.org/bugs/?50619

Do you have a simple example of partial file read?  Say, a four line file in which only two lines are read at a time?

Dan Sebald <sebald>
Tue 26 Sep 2017 07:05:24 AM UTC, original submission:  

Bug # 47671: When textscan reads only part of a file, the read pointer is left at the wrong position, so that subsequent reads re-read some of the data.

Though this bug is reported as patched and closed, it still seems to be present in the Windows build. I am using the Windows 10, 64 bit and get exactly the problem as described when using subsequent textscan() calls.

Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #41897:  read_file.m added by cees_meijer (227B - application/octet-stream - Data file and test program that show the textscan problem)
file #41898:  TestProfiles_1.TXT added by cees_meijer (855B - text/plain - Data file and test program that show the textscan problem)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by hardy (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by lostbard (Posted a comment)
  • -email is unavailable- added by cees_meijer (Posted a comment)
  • -email is unavailable- added by sebald (Posted a comment)
  • -email is unavailable- added by None (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 group members can vote.

     

    Follow 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-10-26 rik5 StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2017-10-21 rik5 StatusNone Patch Submitted
    2017-10-21 rik5 Dependencies- bugs #50619 is dependent
    2017-09-27 sebald Attached File- Added octave-textscan_stream_pointer_restore-djs2017feb27.patch, #41900
        Attached File- Added octave-reuse_delimited_stream_constructor-djs2017feb27.patch, #41901
    2017-09-26 cees_meijer Attached File- Added read_file.m, #41897
        Attached File- Added TestProfiles_1.TXT, #41898

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code