bugGNU Octave - Bugs: bug #46645, Can't load struct variables that...

 
 

bug #46645: Can't load struct variables that use arbitrary fieldnames

Submitter:  Rik <rik5>
Submitted:  Wed 09 Dec 2015 07:38:00 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  3 - Low Item Group:  Regression
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * other Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 27 Mar 2020 08:27:28 PM UTC, comment #14: 

Yes, I have applied this fix to the stable branch as well.

Mike Miller <mtmiller>
Group Member
Sun 15 Mar 2020 09:05:26 PM UTC, comment #13: 

I reviewed, amended, and checked in Olaf's changeset here https://hg.savannah.gnu.org/hgweb/octave/rev/ef349f5c320b.

The patch was old enough that it it didn't apply cleanly to the current code base so I fixed that up.  I also changed the variable name "string_identifier" to "do_name_validation" so that the code can read naturally as "if (do_name_validation && ...)".

The BIST test was ambiguous in that it used whatever save format was set as the default.  I changed the BIST test to always use '-text' format.

@Mike: I applied this to the dev branch because this doesn't seem like a critical issue, and stable is supposed to be finalizing for a 6.1 release.  But if you feel it is important you can 'hg graft' it over to stable.

Marking bug fixed and closing report.

Rik <rik5>
Group administrator
Fri 13 Mar 2020 09:54:27 PM UTC, comment #12: 

Yeah, like I said, the patch itself looks good to me, fixes the problem, and looks ready to commit as far as I can tell, but I'm not sure what needs to be tested to make sure it doesn't disable desirable error checking in other cases.

I tested by writing a simple scalar variable to a file, then edited the file to change the variable name to an invalid one, and an error was still reported when I tried to load it. So far as I can tell, the change only affects names inside structs.

Mike Miller <mtmiller>
Group Member
Fri 13 Mar 2020 08:16:37 PM UTC, comment #11: 

I didn't realize that Olaf had made a potential fix.  I can review it, sometime.  Maybe this weekend, but it might take me longer.

Rik <rik5>
Group administrator
Fri 13 Mar 2020 06:44:14 PM UTC, comment #10: 

Rik, you mentioned earlier that some work would be needed to differentiate recursive calls when loading a struct from top-level calls to load normal variables.

Olaf has posted a patch to bug #50831 that looks like it addresses this exact description of a fix.

The patch works for me on the stable branch (minor fixup needed), fixes the latest example posted here by deego, and works for my cases. But I'm not aware of all of the counterexamples that we want to continue to error about. Is an error for a single top-level variable with an invalid name in a text file enough to demonstrate that we are still erroring when appropriate?

In short, I would like to apply Olaf's patch to fix both bugs, but I don't know enough to know if it's complete or if some cases are missing.

Mike Miller <mtmiller>
Group Member
Fri 13 Mar 2020 06:21:44 PM UTC, comment #9: 

Rik,

Ah, thanks for telling me about isvarname. With that, I was able to find a workaround.
dave

deego <deego>
Fri 13 Mar 2020 05:53:22 PM UTC, comment #8: 

Yes, this is expected at this point.  Someone has to have a definitive interest in fixing this bug and provide a first attempt at a patch.  In the meantime, you can always use isvarname() to guarantee that the variable names you want to use will be acceptable to Octave upon a later re-load.

Rik <rik5>
Group administrator
Fri 13 Mar 2020 04:20:21 AM UTC, comment #7: 

In 2020, in octave 5.2.0, the problem still remains:


Octave (5.2.0) fails to load a structure from a file that it saved previously:

name="^ab^"
aa.(name)=1
aa =
  scalar structure containing the fields:
    ^ab^ =  1

save("/tmp/myfile.octave", "aa")
load("/tmp/myfile.octave", "aa")

=>
error: load: invalid identifier '^ab^' found in file ''

deego <deego>
Thu 15 Dec 2016 04:08:25 PM UTC, comment #6: 

This is another symptom of this bug


x = struct("a.b",1)
x =

  scalar structure containing the fields:

    a.b =  1

save "struct_bug.dat" "x"
clear all
load "struct_bug.dat"
error: load: invalid identifier 'a.b' found in file ''


The file can be written but it can't be loaded. That shouldn't happen.

This is using 4.3.0+ (dev) version.

Juan Pablo Carbajal <juanpi>
Group Member
Thu 10 Dec 2015 08:43:26 PM UTC, comment #5: 

More extensive surgery will be needed.  The function read_text_data() is called, essentially recursively, when trying to load a struct variable.  The first time through the function it identifies that the type is a struct and then calls read_text_data for each of the "sub-variables" which are the fields of the struct. 

The obvious way around this is to get the variable name, and then delay input validation, and get the variable type.  After that one could decide whether to validate the variable name fully or not.  But, we would also need to change the function prototype for read_text_data (and the others like read_binary_data) so that we could pass down a flag to indicate that we are still in a struct.

The backtrace is


#0  read_text_data (is=..., filename=..., global=@0x7fffd693ad4e: false, tc=..., count=0)
    at libinterp/corefcn/ls-oct-text.cc:255
#1  0x00007ffff6ca95ea in octave_scalar_struct::load_ascii (this=0x7fffd04bb9e0, is=...)
    at libinterp/octave-value/ov-struct.cc:1401
#2  0x00007ffff7067c26 in octave_value::load_ascii (this=0x7fffd693b0b0, is=...)
    at ./libinterp/octave-value/ov.h:1264
#3  0x00007ffff7067323 in read_text_data (is=..., filename=..., global=@0x7fffd693b1df: false, tc=...,
    count=0) at libinterp/corefcn/ls-oct-text.cc:281
#4  0x00007ffff703c7a5 in do_load (stream=..., orig_fname=..., format=...,
    flt_fmt=oct_mach_info::flt_fmt_unknown, list_only=false, swap=false, verbose=false, argv=...,
    argv_idx=2, argc=2, nargout=0) at libinterp/corefcn/load-save.cc:368
#5  0x00007ffff703e6a3 in Fload (args=..., nargout=0) at libinterp/corefcn/load-save.cc:856


Incidentally, the problem with the filename not being printed is caused by the fact that we vector through load_ascii() in ov.h which has the following prototype.


  bool load_ascii (std::istream& is) { return rep->load_ascii (is); }


Because we are passing through only the istream we don't have the filename when we actually call read_text_data again.  The prototype for read_text_data is shown below.


read_text_data (std::istream& is, const std::string& filename, bool& global,
                 octave_value& tc, octave_idx_type count)


The simplest thing is probably to change the reporting and don't report information about the file name.  This is fine for interactive use where the load command is proximate in time to the error message.  For scripts, it's not as great because there might be multiple load commands in an m-file and you would need to set debug_on_error(1) in order to figure out which load command generated an error.

Other possibilities include changing the function prototypes to use octave_stream which has the name of the file as a member of the class.  Or we could stick with istream, but change the function prototype of load_ascii to have an additional argument.


Rik <rik5>
Group administrator
Wed 09 Dec 2015 09:37:16 PM UTC, comment #4: 

Ah, except that message is also about normal variables, not just struct fields, so more extensive surgery might be needed.

John W. Eaton <jwe>
Group administrator
Wed 09 Dec 2015 09:24:25 PM UTC, comment #3: 

Yes, let's eliminate that bogus error message.

John W. Eaton <jwe>
Group administrator
Wed 09 Dec 2015 08:16:34 PM UTC, comment #2: 

Attached is a simple file (bad_struct.var) that contains a struct with a single invalid field name.  When I try to load it I get the following.


load bad_struct.var
error: load: bogus identifier '1A' found in file ''


First, there is a problem because the filename should be reported in the error message.  I'll leave that aside for the time being.

Grep'ing for "bogus identifier" shows the only instance is in corefcn/ls-oct-text.cc at line 254.


if (! (name == ".nargin." || name == ".nargout."
       || name == CELL_ELT_TAG || valid_identifier (name)))
  error ("load: bogus identifier '%s' found in file '%s'",
         name.c_str (), filename.c_str ());


If you like, we could delete the call to valid_identifier() and then presumably the file would load.



(file #35680)

Rik <rik5>
Group administrator
Wed 09 Dec 2015 08:00:34 PM UTC, comment #1: 

This change was intentional:

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

As I recall, this wasn't a change I really wanted to make, but one I eventually agreed to do so that people could use structs more like dictionaries.

Maybe what needs to be fixed is the syntax used when saving and loading structs?

John W. Eaton <jwe>
Group administrator
Wed 09 Dec 2015 07:38:00 PM UTC, original submission:  

In version 3.6.4


octave:1> str = "1a"
str = 1a
octave:2> x.(str) = 1
error: invalid structure field name '1a'


in version 3.8.2 and all later versions


octave:1> str = "1a"
str = 1a
octave:2> x.(str) = 1
x =

  scalar structure containing the fields:

    1a =  1


The dynamically created field name for the struct is no longer validated that it is an appropriate variable name.  This is a problem because it is then rather difficult to get the value from the structure because the parser emits an error.  If the struct is saved, the file will no longer load back because load() detects that the fieldname is invalid.


octave:5> x.1a
parse error:

  syntax error

>>> x.1a



Rik <rik5>
Group administrator

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #35680:  bad_struct.var added by rik5 (165B - 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 deego (Posted a comment)
  • -email is unavailable- added by juanpi (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by rik5 (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 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-03-15 rik5 StatusConfirmed Fixed
        Open/ClosedOpen Closed
    2020-03-13 rik5 Priority5 - Normal 3 - Low
    2015-12-10 rik5 Summaryx.(string) doesn\'t validate that string is valid variable name Can't load struct variables that use arbitrary fieldnames
    2015-12-09 rik5 Attached File- Added bad_struct.var, #35680
        StatusNone Confirmed
    2015-12-09 rik5 Summaryx.(string) doesn\'t validate that string is valid variable name. x.(string) doesn't validate that string is valid variable name

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code