bugGNU Octave - Bugs: bug #55577, textscan should reject any...

 
 

bug #55577: textscan should reject any single-precision arg as invalid (file-io.cc-tst BIST test failure)

Submitter:  Hartmut <hardy>
Submitted:  Fri 25 Jan 2019 09:46:19 PM UTC
   
 
Category:  Octave Function Severity:  4 - Important
Priority:  5 - Normal Item Group:  Missed Error or Warning
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * 5.0.90 Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 30 Jan 2019 09:01:40 PM UTC, comment #16: 

This bug is marked as fixed.  Any reason not to close it?

Rik <rik5>
Group administrator
Wed 30 Jan 2019 07:45:07 PM UTC, comment #15: 

AFAICT that is the code that is being discussed here. The textscan function calls lookup to handle its file descriptor argument. Both the stream_list::lookup and stream_list::get_info functions now correctly error if the argument is a single precision value.

Mike Miller <mtmiller>
Group Member
Wed 30 Jan 2019 07:36:13 PM UTC, comment #14: 

Is there some reason that we can't just use the same code that is used by other functions like fread that work with file IDs?


  octave::stream_list& streams = interp.get_stream_list ();

  octave::stream os = streams.lookup (args(0), "fread");


The lookup function allows lookup by name or number.  Using this same approach would make it consistent.  If there is something wrong with finding the file this way, then I think it should be addressed in the same way for all functions that work with file IDs.

John W. Eaton <jwe>
Group administrator
Wed 30 Jan 2019 07:12:25 PM UTC, comment #13: 

So we could further make this even more precise with a check like


  if (! (fid.is_double_type () && fid.is_scalar_type ()))
    ::error ("file id must be ... however we want to describe this");


Mike Miller <mtmiller>
Group Member
Tue 29 Jan 2019 09:31:02 PM UTC, comment #12: 

Ah, thanks, I didn't realize that a file handle is just a double with an integer value. I vaguely assumed there was a bit more magic involved and that we could use "ishandle", but no.

Sorry for the noise.

Philip Nienhuis <philipnienhuis>
Group Member
Tue 29 Jan 2019 09:20:27 AM UTC, comment #11: 

Answering comment #10 with additions:


>> fid = fopen ('existing-text-file.txt')
fid =

     3
>> class (fid)
ans =

    'double'
>> fopen (fid)          % no output == no 'ans'
>> fopen (double (fid)) % no output == no 'ans'
>> fopen (single (fid))
Error using fopen
First input must be a file name or a file identifier.
>> fopen (int32 (fid))
Error using fopen
First input must be a file name or a file identifier.
>> fopen (int64 (fid))
Error using fopen
First input must be a file name or a file identifier.
>> fopen (uint64 (fid))
Error using fopen
First input must be a file name or a file identifier.
>> fgetl (double (fid))
ans =

    -1
>> fgetl (single (fid))
Error using fgets
Invalid file identifier. Use fopen to generate a valid file identifier.

Error in fgetl (line 32)
[tline,lt] = fgets(fid);
>> fgetl (int32 (fid))
Error using fgets
Invalid file identifier. Use fopen to generate a valid file identifier.

Error in fgetl (line 32)
[tline,lt] = fgets(fid);


Quoting "help fopen" (not the same as the online documentation):

> FID is a scalar MATLAB integer valued double, called a file identifier. You use FID as the first argument to other file input/output routines, such as FREAD and FCLOSE. If fopen cannot open the file, it returns -1.


So I think "double" is the only valid file handle data type in Matlab.

Kai Torben Ohlhus <siko1056>
Group Member
Mon 28 Jan 2019 08:30:17 PM UTC, comment #10: 

Unfortunately, "file handle" is a double precision numeric value.

Or are you saying we should accept any class of numeric value if that value can be used to access an open file?

What does the following code do in Matlab (based on comment #, I'm guessing it fails unless the file ID is a double)?


fid = fopen ('existing-text-file')
class (fid)
fopen (fid)
fopen (double (fid))
fopen (single (fid))
fopen (int32 (fid))
fgetl (double (fid))
fgetl (single (fid))
fgetl (int32 (fid))
...


First, does a file ID in Matlab have class "double"?

Second, if passing an intN value to a function like fgetl or fopen works, does it work for all numeric types?  Or, if it fails, does it fail for all types other than double?

The above code works in Octave, so at least fopen and fgetl don't seem to care about the type of the argument as long as the numeric value of the file ID can be used to find the corresponding internal file object.

If the above code does not work in Matlab, then we could consider breaking compatibility with older versions of Octave for better compatibility with Matlab.  But I would prefer to not do that, and to also have consistent behavior for all functions that work with file IDs.

John W. Eaton <jwe>
Group administrator
Mon 28 Jan 2019 07:45:14 PM UTC, comment #9: 

Wouldn't it be better to verify if textscan's first input argument is either a string or a file handle, rather than disqualifying numeric types?

Philip Nienhuis <philipnienhuis>
Group Member
Sun 27 Jan 2019 07:36:05 PM UTC, comment #8: 

This is fixed on the stable branch, single precision file descriptors are now rejected again, for textscan and for other file I/O functions

https://hg.savannah.gnu.org/hgweb/octave/rev/c18e59476795

Mike Miller <mtmiller>
Group Member
Sat 26 Jan 2019 07:32:28 PM UTC, comment #7: 

Thanks, it looks to me like this test is correct and Octave textscan is wrong. I think the input validation should be fixed, and the test should be fixed to go back to using file descriptor 4 instead of 40.

Mike Miller <mtmiller>
Group Member
Sat 26 Jan 2019 01:08:14 PM UTC, comment #6: 

In reply to comment #1 :

  • When I do (in the Octave GUI) cd before I start the individual test, then this tests fully PASSes fine.


  • When I start the Octave GUI directly with the binary (and having an Octave current directory where I have write access to, so that fntest.log can be written), and then run the full test suite, then all tests PASS now !


  • When I start the Octave GUI (from the same directory as above) with a directly calling the run-octave script (mostly as I did in commment #0), then the test also all PASS.


  • Only when I go (with the linux console) to a folder (like /opt/octave-5.0.90 in my case) where I have write access, but no write access to the parent folder (the very same thing as I did in comment #0), then also all tests PASS.


Result: Today I am NOT ABLE TO REPRODUCE this behavior myself. No new compiliation. Just a PC restart inbetween.

Hartmut <hardy>
Sat 26 Jan 2019 11:48:32 AM UTC, comment #5: 

Matlab 2019a prerelease:

:
>> c = textscan(single(fid), '%s')
Error using textscan
First input must be a valid file-id or non-empty character vector.


Philip Nienhuis <philipnienhuis>
Group Member
Fri 25 Jan 2019 10:48:33 PM UTC, comment #4: 

I can now reproduce this failure by forcing a large number of open files.


>> test libinterp/corefcn/file-io.cc-tst
PASSES 90 out of 90 tests
>> fid = [];
>> for i = 1:100; fid(i) = fopen (tempname (), "w+"); endfor
>> test libinterp/corefcn/file-io.cc-tst
ans =
{
  [1,1] = [](0x1)
}

***** error textscan (single (40))
!!!!! error failed.
Expected an error, but got no error


Mike Miller <mtmiller>
Group Member
Fri 25 Jan 2019 10:39:09 PM UTC, comment #3: 

Can someone with access to Matlab verify whether a single precision value is acceptable as a file descriptor to the textscan function?


fname = tempname();
fid = fopen(fname);
c = textscan(single(fid), '%s');


If not, then we should probably make the input validation stricter again and keep the test, and probably reduce the numbers back to 4 instead of 40. My guess is whoever worked on the translation from m-file to C++ didn't understand what these input validation tests were testing for.

It it is acceptable, then we should keep the code as it is and delete the test.

Mike Miller <mtmiller>
Group Member
Fri 25 Jan 2019 10:34:04 PM UTC, comment #2: 

I also think this particular test might be outdated.

These input validation tests


%!error textscan (single (40))
%!error textscan ({40})


have been in Octave since 3.6.4 (with the value 4 instead of 40). They used to both return an error about the type of the argument. But starting with Octave 4.2, the input validation changed and the error message changed. This is also when textscan was rewritten as a compiled function.

Octave 4.0:


>> textscan (single (40))
error: textscan: first argument must be a file id or character string
error: called from
    textscan at line 108 column 5


Octave 4.2 and later:


>> textscan (single (40))
error: textscan: invalid stream number = 40


Maybe you have enough open files in your Octave session that 40 is a valid file descriptor, and the test is not producing an error. Maybe having open tabs in the file editor counts against the file descriptor count, so this number may be higher?

So really I think this test should be deleted, if textscan now accepts a single argument as a valid file descriptor.

Mike Miller <mtmiller>
Group Member
Fri 25 Jan 2019 09:56:22 PM UTC, comment #1: 

Can you please try with


cd /opt/octave-5.0.90/share/octave/5.0.90/etc/tests/libinterp/corefcn
test file-io.cc-tst


This is slightly different behavior in 5 vs 4.4, when using '__run_test_suite__', Octave now does a chdir first to the parent directory of the test file. I've already reported bug #55565 for a couple different tests that are failing if the installation directory is not writeable or not owned by the user running Octave.

FWIW I am not able to reproduce this using your steps to reproduce.

Oh also, you are testing with './run-octave' after doing 'make install'? What if you run '/opt/octave-5.0.90/bin/octave --gui' and then run the test suite, any change?

Mike Miller <mtmiller>
Group Member
Fri 25 Jan 2019 09:46:19 PM UTC, original submission:  

I have observed a failing test, when running "__run_test_suite__" in the Octave GUI:

>>>>> processing /opt/octave-5.0.90/share/octave/5.0.90/etc/tests/libinterp/corefcn/file-io.cc-tst
***** error textscan (single (40))
!!!!! error failed.
Expected an error, but got no error


I can only provoke this failing test when using the Octave GUI, and when using the run_test_suite script. The test does NOT fail when

  • doing "__run_test_suite__" in the Octave CLI (without Octave GUI)
  • doing "make check" from the linux command line
  • when running the individual test from within the Octave GUI several times, like this:


for i = 1:30, disp(i), test /opt/octave-5.0.90/share/octave/5.0.90/etc/tests/libinterp/corefcn/file-io.cc-tst , endfor


Here are details what I did to get Octave and the failing test:

  • Use Ubuntu 18.04 linux
  • Download the current 5.0.90 tar ball alpha.gnu.org
  • extract the tar content to /opt/octave-5.0.90
  • ./configure --prefix=/opt/octave-5.0.90
  • make -j 2
  • make install
  • ./run-octave --gui
  • _run_test_suite_


I see this failing test every time I do this. It's not just on the second round (like another fixed bug report on file-io.cc).

Hartmut <hardy>

 

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

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by siko1056 (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by hardy (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 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-02-01 rik5 Open/ClosedOpen Closed
    2019-01-28 jwe Open/ClosedClosed Open
    2019-01-27 mtmiller StatusConfirmed Fixed
        Open/ClosedOpen Closed
    2019-01-26 mtmiller CategoryTest Suite Octave Function
        Severity3 - Normal 4 - Important
        Item GroupNone Missed Error or Warning
        StatusNeed Info Confirmed
        Summarytest in file-io.cc-tst fails on Octave 5.0.90 (only in GUI with run_test_suite) textscan should reject any single-precision arg as invalid (file-io.cc-tst BIST test failure)
    2019-01-25 mtmiller StatusWorks For Me Need Info
    2019-01-25 mtmiller StatusNone Works For Me

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code