bugGNU Octave - Bugs: bug #59238, load variables from file: the next...

 
 

bug #59238: load variables from file: the next saving operation produces a faulty file

Submitter:  T Knauss <tknauss>
Submitted:  Thu 08 Oct 2020 09:25:04 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Unexpected Error or Warning
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * 6.0.90 Operating System:  * Microsoft Windows
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 25 Oct 2020 08:49:33 AM UTC, comment #29: 

I verified - with the patch applied - that variables saved in text format on Windows are read correctly even if the variables are strings containing line ending characters.

I pushed the patch here:
https://hg.savannah.gnu.org/hgweb/octave/rev/cc287e633588

IIUC that change should be save because the files written on Linux platforms shouldn't be affected by this change. The files saved on all (other) platforms will be the same as the ones saved on Linux.

Closing as fixed.

Markus Mützel <mmuetzel>
Group administrator
Fri 23 Oct 2020 10:45:58 PM UTC, comment #28: 

I verified the patch doesn't produce an regressions on Linux.  I don't have a Windows box that I can test on to see if it actually resolves the errors.  I agree with you that this should go on stable.  It seems bad that Octave can't save and load it's own variables.

Rik <rik5>
Group administrator
Fri 23 Oct 2020 05:07:22 PM UTC, comment #27: 

With the attached patch, files are opened in binary mode for saving variables independent on the file format.

Should we make that change on stable? IIUC, we shouldn't have saved variables in text mode. So I'd say this should go on stable.
Is that ok given that we are coming closer to a release?

(file #50075)

Markus Mützel <mmuetzel>
Group administrator
Wed 21 Oct 2020 09:40:42 PM UTC, comment #26: 

If I understand the implications correctly, it will still be possible to load old files. We've always(?) read in binary mode anyway.
Character vectors with line breaks are problematic when they are stored in files with CRLF line ending. To be able to correctly read such character vectors, the line endings in the file have to be (manually) converted from CRLF to LF by the user.
Files saved after the proposed change won't need this manual conversion.

Markus Mützel <mmuetzel>
Group administrator
Wed 21 Oct 2020 09:04:35 PM UTC, comment #25: 

Whatever you decide, it would be best to remember Postel and retain or gain the ability to load existing files that were created from older versions of Octave, with any style of line ending.

Mike Miller <mtmiller>
Group Member
Wed 21 Oct 2020 08:53:13 PM UTC, comment #24: 

I'm not a fan of translating CRLF back and forth between saving and loading.  I think using binary mode is the way to go.

Rik <rik5>
Group administrator
Wed 21 Oct 2020 07:59:25 PM UTC, comment #23: 

Oh, I see.

Yes, I agree, just use binary mode.  Should we then also do our on writing of system-specific LF/CR/CRLF line endings or just always write LF?  I suppose it would be best to just write LF?

If only I had been more open to just using a binary format for Octave files way back when...

John W. Eaton <jwe>
Group administrator
Wed 21 Oct 2020 07:27:44 PM UTC, comment #22: 

The correct number of characters is stored in the file. The issue here is for strings that contain line feed characters "\n". When those are written to a file that is opened in text mode on an OS that has CRLF default line endings (like Windows) those "\n" characters expand to "\r\n".
When we are loading strings from files, we are reading the number of bytes marked in the file. If some of those bytes expanded to two bytes, we are going to miss some bytes from the end.
If a tool would replace the "\r\n" with "\n" the byte count would be correct again.

If we can't open the files in text mode for reading, that leaves us with options 1.b) (write in binary mode). Or option 2 (adapt the number of read bytes on platforms with CRLF line ending according to how many line endings have been read in the string).
I still prefer option 1.b).

Markus Mützel <mmuetzel>
Group administrator
Wed 21 Oct 2020 07:18:45 PM UTC, comment #21: 

Some of the load_ascii functions use tellg/seekg (or equivalent) on the input stream.  Unless we can avoid those positioning commands, we can't open the files in text mode.

I thought that any place where we stored text we also stored the number of characters.  Is that not correct?

If a tool simply replaces LF with CRLF, couldn't that cause trouble if we have saved strings that have embedded LF characters?


John W. Eaton <jwe>
Group administrator
Wed 21 Oct 2020 06:24:21 PM UTC, comment #20: 

OK. I don't know what was going on yesterday. But it looks like I had my eyes closed when looking at the code...
The binary mode is conditional in `load_save_system::save` in "load-save.cc":

        if (format.type () == BINARY
#if defined (HAVE_HDF5)
            || format.type () == HDF5
#endif
            || format.type () == MAT_BINARY
            || format.type () == MAT5_BINARY
            || format.type () == MAT7_BINARY)
          mode |= std::ios::binary;


So "-text" uses text mode for saving. No wonder there are CRLF line endings.

The files are opened in binary mode unconditionally in `load_save_system::load`. No wonder there are additional CRs in the recovered string.

Possible options are:
1. Open the file in the same mode for writing and reading. Two possibilities:
  a) Keep the conditional binary mode in `load_save_system::save` and use the same mode (with the same condition) in `load_save_system::load`. That would lead to consistent results if the files are saved and loaded on the same OS. The issue would persist if files saved on Windows would be loaded on Linux (as an example for two OSs that use different default line endings).
  b) Always use binary mode unconditionally. That would lead to consistent results also when exchanging files between OSs with different default line endings. Loading strings with line breaks from broken files (i.e. ones with CRLF line ending) would continue to be broken.
2. Add additional logic that counts CR characters removes them from the string and continues to read the missing characters from the save file.

I'd prefer to follow on with option 1.b).
With option 2.a) we'd continue to have issues when exchanging files between OSs with different default line ending.
Option 2 seems messy and would complicate the code unnecessarily.
It is easy to "repair" the invalid files that Octave produces until now by replacing CRLF line endings with LF line endings with third party tools.

What do you think?

Markus Mützel <mmuetzel>
Group administrator
Wed 21 Oct 2020 05:41:27 PM UTC, comment #19: 

I don't remember what exactly I did yesterday. But today I see the exact same result on Octave 5.2, on Octave 6.0.90 and the cross-build from last week.
I probably got my test scripts with "-text" and "-binary" mixed up. I'm sorry for the wrong information.

Correction: This is still not fixed on default.

I am still wondering why the file is written with CRLF line ending when the ofstream is opened in binary mode...

Markus Mützel <mmuetzel>
Group administrator
Wed 21 Oct 2020 02:52:44 AM UTC, comment #18: 

Maybe it is worth bisecting to determine the change that fixed things.  If it is just a single changeset then it would probably make sense to backport it to the stable branch.

Rik <rik5>
Group administrator
Tue 20 Oct 2020 07:09:41 PM UTC, comment #17: 

The only change in that time frame that is remotely related is this one:
https://hg.savannah.gnu.org/hgweb/octave/rev/159b6a1eb408

But I fail to understand how that could have any influence on line endings.
Since that change, we use the constructor of `ifstream` and `ofstream` with `wchar_t` instead of `char`:
https://docs.microsoft.com/en-us/cpp/standard-library/fstream-typedefs?view=vs-2019#ifstream
https://docs.microsoft.com/en-us/cpp/standard-library/basic-ifstream-class?view=vs-2019#basic_ifstream

The stream itself should be the same...

Markus Mützel <mmuetzel>
Group administrator
Tue 20 Oct 2020 06:42:50 PM UTC, comment #16: 

@jwe: I crossed with your comment #14. A file that doesn't load correctly is file #50030.

I previously tested with Octave 5.2.0.
Running the test from comment #9 again with the release candidate Octave 6.0.90, it also fails.
But with a cross-build from the default branch from last week, it is working fine. The file saved with "-text" still contains CRLF line endings. But all characters are read correctly.

I wonder which change fixed this...

Markus Mützel <mmuetzel>
Group administrator
Tue 20 Oct 2020 06:42:13 PM UTC, comment #15: 

@Markus: Thanks, that confirms what I was thinking and what you have deciphered as well.

The string in Octave is 15 characters long.  When Octave writes this data to a file it does so in binary format and records the length.  For our test string, the output is


# type: string
# elements: 1
# length: 15


However, there is some unnecessary newline processing and "\n" is being converted to CRLF.  Since the string had three newlines in it the actual length written to the files was 18 characters.

If you read this back in with the specified length of 15 then you truncate 3 characters from the string.

In my test, I simply hacked the length to 18 to see if that was enough to have it read the whole string, which it is.

One solution would be to find out where the "\n" -> CRLF expansion is happening and suppress it so that only one character is ever written.  Otherwise, we have to adjust both the length of the string as stored in the file and have code in the load routine to unconvert CRLF back to "\n".  That sounds harder, but I haven't looked at it deeply.

Rik <rik5>
Group administrator
Tue 20 Oct 2020 06:29:20 PM UTC, comment #14: 

All characters are read. But the size of the loaded string is wrong:

>> size (cell_str{1})
ans =
    1   18
>> size (cell_str2{1})
ans =
    1   15


It contains the CRLF line endings where it probably should only contain LF line endings like the original string `cell_str2`.

I'm looking at the code in `load-save.cc`:
We are opening the ifstream when loading from a file in binary mode (line 1317). That means that all bytes (including the CRLF) will be read as they are stored in the file on all platforms. The comment above suggests that this was a conscious decision.
But it also looks like we open the ofstream for saving the data to a file in binary mode as well (line 1479). IIUC, that means that all bytes should be written as is to the output stream. Only if we'd be opening the stream in text mode, the line ending would be converted to CRLF on Windows (see e.g. [1]).
I wonder where the CRLF come from in the first place. They should not be there...

[1]: https://en.cppreference.com/w/cpp/io/c/FILE#Binary_and_text_modes

Markus Mützel <mmuetzel>
Group administrator
Tue 20 Oct 2020 06:26:46 PM UTC, comment #13: 

I agree that we should use binary formats and that for internal purposes we should be specifying the format rather than leaving it up to the prevailing default.

The -text format on Windows should work, however.  We should always be opening these files internally in Octave in binary mode and dealing with the line endings ourselves rather than relying on text mode conversions because we need to use ftell/fseek (or equivalent C++ stream operations) to save and restore positions while parsing the files.  So if that is not happening correctly, it is a bug.  If you have examples of files that can't be saved/loaded properly, please point me to them.  I have not been following this report closely but I would like to make -text mode work correctly if possible.

John W. Eaton <jwe>
Group administrator
Tue 20 Oct 2020 05:36:53 PM UTC, comment #12: 

I think we should make -binary the default save format for packages, and it is likely to be smaller than the equivalent file saved as -text.  That should be a separate project/bug report though.

I've uploaded a test file cell_str_text.testing.txt.  Could you run the following code to check it?


cell_str2 = {"This\nis\na\ntest."};
load cell_str_text.testing.txt
isequal (cell_str, cell_str2)




(file #50032)

Rik <rik5>
Group administrator
Tue 20 Oct 2020 05:30:57 PM UTC, comment #11: 

Saving and loading with "-binary" works.

Attached the saved files in "-text" and "-binary" format.

(file #50030, file #50031)

Markus Mützel <mmuetzel>
Group administrator
Tue 20 Oct 2020 05:16:48 PM UTC, comment #10: 

@Markus: Thanks for creating a minimal example.  Can you try using binary format?  This should resolve the issue.  If it does, it would also be a simple patch for the symbolic package to use the "-binary" option.  There is certainly an issue to address here, but it is also a failing code pattern to rely on defaults (like -text with only LF endings) when those defaults are not guaranteed.

Simple test:


cell_str = {"This\nis\na\ntest."};
save cell_str_text.txt cell_str -binary
cell_str2 = cell_str;
load cell_str_text.txt
isequal (cell_str, cell_str2)


Also, could you upload the file "cell_str_text.txt"?  I'd like to take a look at the contents and the line endings.


Rik <rik5>
Group administrator
Tue 20 Oct 2020 10:12:28 AM UTC, comment #9: 

Sorry, I misunderstood what I was supposed to test.
Saving a double variable with the following commands, also leads to a file with CRLF line endings on Windows:

A = rand(4);
save A_text.txt A -text


Loading that file (again on Windows) successfully restores all matrix elements of A.

The issue seems to be with saving and loading cell strings in "-text" format if the line ending is CRLF.

An example that reproduces the issue for me on Windows is:

cell_str = {"This\nis\na\ntest."};
save cell_str_text.txt cell_str -text
cell_str2 = cell_str;
load cell_str_text.txt


The length of the string as marked in `cell_str_text.txt` is 15. But because the string was saved with CRLF line endings, 18 bytes have to be read from the file.

I see the following possible solutions:
- When saving, write the string literal in the file with LF line endings. This might be awkward when displaying the file in editors that have issues with mixed line endings in the same file.
- When saving, increase "length" by the number of additional bytes for the CR characters.
- When loading, count CRLF as one byte only.

Afaict, this is an issue in core Octave. There's not much the "symbolic" package can do about it.

Markus Mützel <mmuetzel>
Group administrator
Tue 20 Oct 2020 07:23:01 AM UTC, comment #8: 

In my opinion, Octave should be allowed to use whatever line-ending  it wants to use, which means, any of CR and LF and CRLF should be allowed. You may define a line-ending that should be used, but this does not mean that it has to be used always.

The load operation on the other hand should be as flexible as possible and recognize all 3 types, which should be quite simple. This also would enable exchanging saved files between different operating systems and easy manual editing of the saved file on any operating system.

T Knauss <tknauss>
Mon 19 Oct 2020 09:45:53 PM UTC, comment #7: 

You can see from the attachments that the file is saved in '-text' format, not '-ascii'. I don't remember how Octave's load and save commands hook into user defined classes. So my earlier questions are about how 'save -text' is expected to work on Windows, and does Octave take care of all line ending translations or are user defined classes expected to handle that? It certainly looks like the load command is truncating too many characters when handing some text off to initialize a symbolic variable.

Mike Miller <mtmiller>
Group Member
Mon 19 Oct 2020 09:00:08 PM UTC, comment #6: 

It's still not clear to me what save() command the symbolic package is using.  It might be -ascii, or it might not.

I don't want to go down a false path and investigate the -ascii format if it is not being used.

Rik <rik5>
Group administrator
Mon 19 Oct 2020 07:56:36 PM UTC, comment #5: 

The default line ending on Windows is CRLF. The following example produces a file with CRLF line ending on Windows:

A = rand(4);
save A_test.txt A -ascii


Having written that: It's also quite common to come across files with LF line ending on Windows. Most editors (including in the meantime also the built-in Notepad) display such files just fine.

Markus Mützel <mmuetzel>
Group administrator
Tue 13 Oct 2020 09:20:45 PM UTC, comment #4: 

I don't have the symbolic package nor can I run on Windows.  But, I think I can help narrow down the issue.

What format does the symbolic use to save its variables?  That should be in the load/save routines for the package.  If it is using "-ascii" that would certainly be a problem.  If the save command does not specify anything then it will be whatever the user has set with save_default_options().  That will certainly create portability problems.  It might be best to explicitly specify "-binary" when saving since that format does not depend on whatever value a newline character is.

Rik <rik5>
Group administrator
Mon 12 Oct 2020 09:12:06 PM UTC, comment #3: 

I think the load function truncates the variables when the file in this example has CRLF line endings. If I test with the original attached file, the variables are truncated after loading. If I strip all carriage returns first, then the load and save seem to work correctly.

Is it expected behavior that text format files have CRLF line endings with Octave on Windows? Or is this file considered malformed because it has the wrong line ending? Or if it is valid, is the bug in Octave or in the way the symbolic package creates its variables from text data?

Mike Miller <mtmiller>
Group Member
Fri 09 Oct 2020 11:57:58 AM UTC, comment #2: 

Yes, it also happens from CLI.

T Knauss <tknauss>
Fri 09 Oct 2020 05:32:19 AM UTC, comment #1: 

Thank you for the bug report.

Does this also happen in the command line version of octave? You have chosen "GUI" as category for this bug but I do not think that the GUI is involved here.

Torsten Lilge <ttl>
Group Member
Thu 08 Oct 2020 09:25:04 PM UTC, original submission:  

I am using "symbolic" package 2.9.0 and I have installed python 3.9 with sympy 1.5.1 (sympy 1.6.2 fails to work) on Win 10 Enterprise 1809 x64.

I discovered the failure when saving my symbolic matrices, but I think it's independent from the "symbolic" package.

Run these commands:
load transformationVars
save transformationVars2

Then compare the two files, e.g. with Beyond Compare. You'll see that the last element in many (or all?) matrices is missing.
The files are attached.

T Knauss <tknauss>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #50075:  bug59238_save_binary_mode.patch added by mmuetzel (2KiB - application/octet-stream)
file #50032:  cell_str_text.testing.txt added by rik5 (233B - text/plain)
file #50031:  cell_str_binary.txt added by mmuetzel (110B - text/plain)
file #50030:  cell_str_text.txt added by mmuetzel (233B - text/plain)
file #49945:  transformationVars added by tknauss (23KiB - application/octet-stream)
file #49946:  transformationVars2 added by tknauss (23KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by ttl (Posted a comment)
  • -email is unavailable- added by tknauss (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 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-10-25 mmuetzel StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2020-10-23 mmuetzel Attached File- Added bug59238_save_binary_mode.patch, #50075
        StatusConfirmed Patch Submitted
        Release5.2.0 6.0.90
    2020-10-20 rik5 Attached File- Added cell_str_text.testing.txt, #50032
    2020-10-20 mmuetzel Attached File- Added cell_str_text.txt, #50030
        Attached File- Added cell_str_binary.txt, #50031
    2020-10-12 mtmiller CategoryGUI Octave Function
        StatusNone Confirmed
    2020-10-08 tknauss Attached File- Added transformationVars2, #49946
    2020-10-08 tknauss Attached File- Added transformationVars, #49945

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code