bugGNU Octave - Bugs: bug #65342, replace atoi with stoi

 
 

bug #65342: replace atoi with stoi

Submitter:  None
Submitted:  Wed 21 Feb 2024 04:39:20 PM UTC
   
 
Category:  Coding Style and Maintenance Severity:  1 - Wish
Priority:  4 Item Group:  Feature Request
Status:  Fixed Assigned to:  None
Originator Name:  Originator Email:  -email is unavailable-
Open/Closed:  * Closed Release:  * 7.2.0
Operating System:  * Any Fixed Release:  10.1.0 (current default)
Planned Release:  10.1.0 (current default)
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 04 Apr 2024 06:41:07 PM UTC, comment #8: 

No more mornings and tests pass.  Marking as Fixed and closing report.

Rik <rik5>
Group administrator
Sun 24 Mar 2024 05:01:18 AM UTC, comment #7: 

The unused variable was fixed in https://hg.savannah.gnu.org/hgweb/octave/rev/399be7cc310f

The error function throws an octave::execution_exception, which is not derived from either std::invalid_argument or std::out_of_range, so it won't be caught in the two catch blocks associated with this try block.

I'm not sure what I was thinking when I started that "I'm not sure what the" comment, but it is not wrong, LOL!  Looking at it now, I'm still not sure what the intent is there.  The original code was


if (atoi (arg.c_str ()) == 0)
  {
    // We have class and function names but already
    // stored the class name in fcn_name.
    class_name = fcn_name;
    fcn_name = arg;
    pos++;
    break;
  }


So maybe we should be doing something like


// FIXME: we really want to distinguish number
// vs. method name here.

bool int_conv_ok = true;

try
  {
    if (std::stoi (arg) == 0)
      error ("%s: invalid integer value for ???");
  }
catch (const std::invalid_argument&)
  {
    // Assume we are looking at a function name.
    // We have class and function names but already
    // stored the class name in fcn_name.
    class_name = fcn_name;
    fcn_name = arg;
    pos++;
    break;
  }
catch (const std::out_of_range&)
  {
    error ("%s: invalid integer value for ???");
  }


but I don't understand the reason for attempting the integer conversion if the value will never be used.

There is a comment above this function that says


// FIXME: This function probably needs to be completely overhauled to
// correctly parse the full syntax of the dbstop command and properly
// reject incorrect forms.


Seems accurate to me.

John W. Eaton <jwe>
Group administrator
Sat 23 Mar 2024 09:58:10 PM UTC, comment #6: 

Question on coding style.  In debug.cc I see


+      try
+        {
+          int line = std::stoi (arg);
+
+          if (line <= 0)
+            error ("%s: start and end lines must be >= 1\n", who);
+
+          start = line;
+          end = line;
+        }
+      catch (const std::invalid_argument&)
+        {
+          // May be a name instead of a number.
+          return false;
+        }
+      catch (const std::out_of_range&)
+        {
+          error ("%s: integer value out of bounds while parsing '%s'", who, arg.c_str ());
+        }


In the second try block there is a call to to std::stoi and then a check on line number which calls error().  Will the call to error() in any way be intercepted by the try block?  If that were the case then the error would get swallowed because I don't see a catch block that would grab the execution exception that Octave throws.

At bp-table.cc:443 there is an apparently truncated comment


              // FIXME: I'm not sure what the


I'm getting a warning about an unused variable during compilation now.


libinterp/parse-tree/bp-table.cc: In member function ‘void octave::bp_table::parse_dbfunction_params(const char*, const octave_value_list&, std::string&, std::string&, octave::bp_table::bp_lines&, std::string&)’:
libinterp/parse-tree/bp-table.cc:489:24: warning: unused variable ‘skip’ [-Wunused-variable]
  489 |                   bool skip = false;
      |                        ^~~~


I think this is an accidental inclusion in the cset.




Rik <rik5>
Group administrator
Sat 23 Mar 2024 04:24:30 PM UTC, comment #5: 

I pushed the following changeset on default.  Marking as ready for test.

https://hg.savannah.gnu.org/hgweb/octave/rev/4e5bc9c4f657

John W. Eaton <jwe>
Group administrator
Fri 22 Mar 2024 04:12:00 PM UTC, comment #4: 

I agree with Rik.  Switching to stoi will be more work than just replacing atoi with std::stoi.  For example, in the Fdbtype function in debug.cc, instead of


@@ -654,7 +654,7 @@ numbers.
           }
         else  // (dbtype fcn) || (dbtype lineno)
           {
-            int line = atoi (arg.c_str ());
+            int line = std::stoi (arg);

             if (line == 0)  // (dbtype fcn)
               fcn_name = arg;


we will probably want to do something like


@@ -654,18 +654,27 @@ numbers.
           }
         else  // (dbtype fcn) || (dbtype lineno)
           {
-            int line = atoi (arg.c_str ());
+            int line;

-            if (line == 0)  // (dbtype fcn)
-              fcn_name = arg;
-            else  // (dbtype lineno)
+            try
               {
+                line = std::stoi (arg);
+
                 if (line <= 0)
                   error ("dbtype: start and end lines must be >= 1\n");

                 start = line;
                 end = line;
               }
+            catch (const std::invalid_argument&)
+              {
+                // Not a number: dbtype fcn
+                fcn_name = arg;
+              }
+            catch (const std::out_of_range&)
+              {
+                error ("dbtype: line number out of range")
+              }
           }
       }
       break;


Although this will require a lot more thought and effort to do correctly, I think it will be worth it to be able to distinguish between invalid input, out of range values, and not confuse "0" with an invalid or out of range input.

I thought about the possibility of writing a wrapper around std::stoi, but the action to take when an exception is thrown can vary in each case so it is not clear to me that we can do better than using individual try/catch blocks for each instance.  Fortunately, there are fewer than 20 instances in the Octave sources, so the conversion should be manageable.

John W. Eaton <jwe>
Group administrator
Sun 03 Mar 2024 04:19:18 AM UTC, comment #3: 

The patch works fine when values are normal and within range.  However, if the conversion cannot be completed then C++ throws an exception.  The conversions need to be protected with try/catch blocks in order to prevent an immediate halt of the program.  Here is an example session with the patch applied:


dbtype ls xyz
terminate called after throwing an instance of 'std::invalid_argument'
  what():  stoi
fatal: caught signal Aborted -- stopping myself...
Abort (core dumped)


This is very different from the C function atoi which returns a 0 if the conversion cannot be made.  This value is checked in the existing code to produce a reasonable error message and then a return to the Octave prompt.


dbtype ls xyz
error: dbtype: start and end lines must be >= 1



Rik <rik5>
Group administrator
Sat 02 Mar 2024 07:58:55 PM UTC, comment #2: 

Is attached.

Anonymous
Wed 21 Feb 2024 04:55:01 PM UTC, comment #1: 

This would be a good idea and a fairly simple replacement.  Can you prepare a changeset or patch for this?  There are only 19 instances of atoi in the entire Octave codebase.  See below and in the attached file atoi.lst


libinterp/corefcn/debug.cc:643:            start = atoi (start_str.c_str ());
libinterp/corefcn/debug.cc:647:              end = atoi (end_str.c_str ());
libinterp/corefcn/debug.cc:657:            int line = atoi (arg.c_str ());
libinterp/corefcn/debug.cc:685:            start = atoi (start_str.c_str ());
libinterp/corefcn/debug.cc:689:              end = atoi (end_str.c_str ());
libinterp/corefcn/debug.cc:693:            start = atoi (arg.c_str ());
libinterp/corefcn/debug.cc:754:          n = atoi (s_arg.c_str ());
libinterp/corefcn/debug.cc:801:              n = atoi (s_arg.c_str ());
libinterp/corefcn/debug.cc:950:          n = atoi (s_arg.c_str ());
libinterp/corefcn/debug.cc:1041:          n = atoi (arg.c_str ());
libinterp/corefcn/event-manager.cc:365:                retval(idx++) = atoi (str.c_str ());
libinterp/corefcn/event-manager.cc:380:      retval = ovl (items, *it++, atoi (it->c_str ()));
libinterp/dldfcn/__init_fltk__.cc:606:                    val = atoi (valstr.c_str ());
libinterp/parse-tree/bp-table.cc:389:          else if (atoi (args(pos).string_value ().c_str ()) > 0)
libinterp/parse-tree/bp-table.cc:434:              if (atoi (arg.c_str ()) == 0)
libinterp/parse-tree/bp-table.cc:461:                  int line = atoi (args(pos).string_value ().c_str ());
liboctave/util/data-conv.cc:394:              block_size = atoi (s.c_str ());
liboctave/util/data-conv.cc:461:          block_size = atoi (s.c_str ());
liboctave/util/pathsearch.cc:114:        kpse_debug |= atoi (val.c_str ());



(file #55731)

Rik <rik5>
Group administrator
Wed 21 Feb 2024 04:39:20 PM UTC, original submission:  

Please, replace C style atoi () with C++11 style stoi () for better security and error handling.

Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #55769:  patch.txt added by None (7KiB - text/plain)
file #55731:  atoi.lst added by rik5 (1KiB - 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 (Updated 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
    2024-04-04 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2024-03-23 jwe StatusConfirmed Ready For Test
        Fixed ReleaseNone 10.1.0 (current default)
    2024-03-02 None Attached File- Added patch.txt, #55769
    2024-02-21 rik5 Attached File- Added atoi.lst, #55731
        CategoryOctave Function Coding Style and Maintenance
        Severity3 - Normal 1 - Wish
        Priority5 - Normal 4
        StatusNone Confirmed
        Operating SystemGNU/Linux Any
        Planned ReleaseNone 10.1.0 (current default)

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code