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

 
 

bug #42651: Incorrect input validation for strings

Submitted by:  Rik <rik5>
Submitted on:  Sun 29 Jun 2014 10:57:20 PM UTC  
 
Category: InterpreterSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Inaccurate Result
Status: FixedAssigned to: None
Originator Name: Open/Closed: Closed
Release: devOperating System: GNU/Linux

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Wed 17 Dec 2014 12:23:53 AM UTC, comment #9:

Fixed in this cset http://hg.savannah.gnu.org/hgweb/octave/rev/03067dab10ca and this addendum http://hg.savannah.gnu.org/hgweb/octave/rev/c2f4f6eb5907. Closing report.

Rik <rik5>
Project 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>
Project 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>
Project 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>
Project 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>
Project 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.

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.

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

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

I agree that we should try to be consistent.

Note that

is the same as

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,

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

John W. Eaton <jwe>
Project 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.

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>
Project 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

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>
Project 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

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

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.

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

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>
Project Administrator

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #32662:  isstring.cset added by rik5 (37kB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by jwe (Posted a comment)
  • -unavailable- added by rik5 (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 4 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 17 Dec 2014 12:23:53 AM UTCrik5StatusPatch Submitted=>Fixed
      Open/ClosedOpen=>Closed
    Sun 14 Dec 2014 12:53:10 AM UTCrik5Attached File-=>Added isstring.cset, #32662
    Sun 14 Dec 2014 12:51:15 AM UTCrik5StatusNone=>Patch Submitted

    Back to the top


    Powered by Savane 3.1-cleanup