bugGNU Octave - Bugs: bug #42651, Incorrect input validation for...

 
 

bug #42651: Incorrect input validation for strings

Submitter:  Rik <rik5>
Submitted:  Sun 29 Jun 2014 10:57:20 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Inaccurate Result
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 17 Dec 2014 12:23:53 AM UTC, comment #9: 
Rik <rik5>
Group administrator
Mon 15 Dec 2014 10:41:54 PM UTC, comment #8: 

Rik, I think your change is OK.

I have always intentionally written "character string", but I can go with "string".

John W. Eaton <jwe>
Group administrator
Sun 14 Dec 2014 12:59:44 AM UTC, comment #7: 

The issue with double-quoted strings concatenated with numbers resulting in a single-quoted string is now bug #43815

Rik <rik5>
Group administrator
Sun 14 Dec 2014 12:52:01 AM UTC, comment #6: 

The issue with Octave:str-to-num warning never being issued is now bug #43814.

Rik <rik5>
Group administrator
Sun 14 Dec 2014 12:51:15 AM UTC, comment #5: 

I'm attaching a patch which makes the input validation for strings stricter.  I think it is better to warn programmers when a function is being used incorrectly, rather than to attempt to proceed with bogus input, which nevertheless can be converted to a string value.

I'll split the other two issues uncovered into new bug reports.

Rik <rik5>
Group administrator
Mon 30 Jun 2014 09:39:35 PM UTC, comment #4: 

The 'sq_string' might be because we want to mimic Matlab, but I don't really know.

For sort, I think we should be checking that the second argument is either a numeric scalar (DIM to sort on) or is a string (MODE).  It doesn't need to match the type of the first operand which might be numeric, string, or cellstr.  I know I'm repeating myself, but the option string for a function really should be a string, but doesn't have to have any relationship to what kind of input the function itself acts on.  Given that mixed char/numeric concatenation is promoted to character class by the interpreter people could still have strange obtuse code without generating an error message.


sort (..., ["ascen" 100])


And yes, something is very wrong with the num-to-str warning.  The warning is "on" in 3.2.4 - 4.1.X but the test code from warning_ids() doesn't produce a warning.


[ "f", 111, 111 ]


I think that deserves it's own bug report.  Interestingly, the warning going the other way does work.


warning ('on', 'Octave:str-to-num')
'abc' + 0
warning: implicit conversion from string to real N-d array
ans =

   97   98   99


Rik <rik5>
Group administrator
Mon 30 Jun 2014 08:24:15 PM UTC, comment #3: 

I agree that we should try to be consistent.

Note that


sort ([66, "A", 67], [66 65 67])


is the same as


sort ('BAC', [66, 65, 67])


since the conversion of the mixed numeric and character array happens when the array is constructed.

For this particular example, maybe sort should warn when the two objects don't have the same type?  I would guess this warning would need to be disabled in "traditional" mode.

Also, this brings up a couple of other issues.  We already have an "Octave:num-to-str" warning ID that is supposed to be enabled by default, but I don't get any warning for the mixed numeric and character array construction.

Also,


[ 66, "A", 67 ]


produces an sq_string object, not a dq_string.  I'm not sure why that choice was made.
-verbatim-

John W. Eaton <jwe>
Group administrator
Mon 30 Jun 2014 08:14:36 PM UTC, comment #2: 

I think the general behavior of changing transparently between numeric and char is fine.  But I don't think it makes sense in many of these input validation cases.  The change doesn't restrict the range of inputs that the function can accept, rather it is just restrictive of the options to the function.  This may eliminate an obtuse corner case, but I don't think it would be overly restrictive.

An example with the first function on the list will probably help.


sort ([66, "A", 67])
ans = ABC
-- Fine

sort ([66, "A", 67], [66 65 67])
ans = BAC
-- No warning, different output, what is it doing?
-- I want a clue that something is amiss.

sort ([66, "A", 67], "ascend")
ans = ABC
-- Fine

mode = double ("ascend")
mode =

    97   115    99   101   110   100
sort ([66, "A", 67], mode)
ans = ABC
-- This rather obtuse usage case would be eliminated.
-- But, you could always call sort (..., char (mode))
-- if you felt that you had to do things this way.


In terms of the code base it seems that about 1/2 the instances are already coded with the is_string predicate and half with the check on error_state.  It would probably be best to decide on a single approach and convert the others whichever half needs it. 

Rik <rik5>
Group administrator
Mon 30 Jun 2014 07:23:16 PM UTC, comment #1: 

Maybe it doesn't make sense, but I prefer to use string_value followed by checking for an error in case there is an octave_value type that can legitimately be converted to a string.

The automatic conversion from numbers to strings was done for what I thought was compatibility with Matlab.  But maybe that's no longer needed, or at least not in as many places.  I do know that it is needed for things like


[ 65:67, '123' ]


but maybe what we should do is just change the way string_value works for numeric classes, and force the conversions to happen where necessary for compatibility?  I think that would mean fewer places that need to be changed.

John W. Eaton <jwe>
Group administrator
Sun 29 Jun 2014 10:57:20 PM UTC, original submission:  

A number of input validation code blocks in libinterp are using an incorrect pattern which does not result in the correct validation of string input.

The list of occurrences is


corefcn/data.cc:6549:          error ("sort: MODE must be a string");
corefcn/help.cc:1387:        error ("__list_functions__: DIRECTORY argument must be a string");
corefcn/matrix_type.cc:225:                error ("matrix_type: TYPE must be a string");
corefcn/matrix_type.cc:425:                error ("matrix_type: TYPE must be a string");
corefcn/qz.cc:407:      error ("qz: OPT must be a string");
corefcn/balance.cc:250:          error ("balance: OPT argument must be a string");
corefcn/file-io.cc:578:            ::error ("%s: architecture type must be a string", fcn);
corefcn/file-io.cc:581:        ::error ("%s: file mode must be a string", fcn);
corefcn/file-io.cc:584:    ::error ("%s: file name must be a string", fcn);
corefcn/file-io.cc:925:            ::error ("%s: format TEMPLATE must be a string", who.c_str ());
corefcn/file-io.cc:1264:        ::error ("%s: argument STRING must be a string", who.c_str ());
corefcn/file-io.cc:1388:                    ::error ("fread: ARCH architecture type must be a string");
corefcn/file-io.cc:1397:        ::error ("fread: PRECISION must be a string");
corefcn/file-io.cc:1656:                ::error ("fwrite: ARCH architecture type must be a string");
corefcn/file-io.cc:1665:    ::error ("fwrite: PRECISION must be a string");
corefcn/file-io.cc:1872:            ::error ("popen: MODE must be a string");
corefcn/file-io.cc:1875:        ::error ("popen: COMMAND must be a string");
corefcn/file-io.cc:1937:            ::error ("PREFIX must be a string");
corefcn/file-io.cc:1940:        ::error ("DIR argument must be a string");
corefcn/file-io.cc:2079:        error ("mkstemp: TEMPLATE argument must be a string");
corefcn/schur.cc:150:          error ("schur: second argument must be a string");
corefcn/syscalls.cc:239:        error ("exec: FILE must be a string");
corefcn/syscalls.cc:372:        error ("popen2: COMMAND argument must be a string");
corefcn/sysdep.cc:600:            error ("putenv: VALUE must be a string");
corefcn/sysdep.cc:603:        error ("putenv: VAR must be a string");
corefcn/time.cc:481:        error ("strftime: FMT must be a string");
corefcn/time.cc:530:            error ("strptime: FMT must be a string");
corefcn/time.cc:533:        error ("strptime: argument STR must be a string");
corefcn/toplev.cc:1224:        error ("atexit: FCN argument must be a string");
corefcn/utils.cc:334:        error ("file_in_loadpath: FILE argument must be a string");
corefcn/utils.cc:417:        error ("file_in_path: PATH must be a string");
corefcn/utils.cc:791:        error ("undo_string_escapes: S argument must be a string");
corefcn/variables.cc:184:        error ("%s: expecting first argument to be a string",
corefcn/variables.cc:347:      error ("isglobal: NAME must be a string");
corefcn/variables.cc:2072:        error ("munlock: FCN must be a string");
corefcn/variables.cc:2108:        error ("mislocked: FCN must be a string");
octave-value/ov-fcn-handle.cc:1858:        error ("str2func: FCN_NAME must be a string");
octave-value/ov-java.cc:2062:            error ("javaObject: CLASSNAME must be a string");
octave-value/ov-java.cc:2138:            error ("javaMethod: METHODNAME must be a string");
octave-value/ov-java.cc:2207:            error ("__java_get__: NAME must be a string");
octave-value/ov-java.cc:2269:            error ("__java_set__: NAME must be a string");


The problem is that the code uses this sequence for input validation.


std::string tmp = args(0).string_value ();

if (! error_state)
   ...
else
   error ("ARG must be a string")


But Octave will coerce numeric arrays into strings so despite there being incorrect input the error message is never given.  This code runs in Octave without an error message, but is certainly not what you want.


exec (1)


The fix is to use the is_string() predicate test instead.  The code should look like this


if (args(0).is_string ())
   tmp = args(0).string_value ();
   ...
else
   error ("ARG must be a string")


These are easy changes, but there are so many of them, that I thought I would post this to the bug tracker and also put it on the list of easy bug fixes.  For people who want to experiment with learning Octave's internals and how the C++ code is written this is an easy first step

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 #32662:  isstring.cset added by rik5 (37KiB - 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 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 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2014-12-17 rik5 StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2014-12-14 rik5 Attached File- Added isstring.cset, #32662
    2014-12-14 rik5 StatusNone Patch Submitted

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code