bugGNU Octave - Bugs: bug #54407, [octave-forge](io) Fix string...

 
 

bug #54407: [octave-forge](io) Fix string access in csv2ell

Submitted by:  Orion Poplawski <opoplawski>
Submitted on:  Sun 29 Jul 2018 11:32:03 PM UTC  
 
Category:  Octave Forge Package Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Segfault, Bus Error, etc.
Status:  Fixed Assigned to:  Philip Nienhuis <philipnienhuis>
Originator Name:  Open/Closed:  Closed
Release:  other Operating System:  Any

Add a New Comment (Rich Markup)
   

You are not logged in

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

 

Fri 03 Aug 2018 09:17:25 PM UTC, comment #5: 

I pushed your patch here after checking the built-in tests still pass:
http://hg.code.sf.net/p/octave/io/rev/d7f9bd65d2c6

As to the other modules, I didn't see any other occurrences of improper string indexing, which is not to say they aren't there :-) just that I didn't spot them.

Thanks for the patch, closing report.

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Tue 31 Jul 2018 08:14:06 AM UTC, comment #4: 

OK, I'll look into it. AFAIR (memory comes back slowly) there was something with the way the string length was tested this way, maybe EOLs or so (CR-LF).

Amyway thanks for reporting.

Title adapted (again)

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Mon 30 Jul 2018 08:18:50 PM UTC, comment #3: 

Well, no - the issue is you are indexing str[k+1], and you need to make sure that k+1 < the length of str.

Orion Poplawski <opoplawski>
Mon 30 Jul 2018 08:16:27 PM UTC, comment #2: 

Tests were run, but they didn't trigger the error.  They were triggered in the octave-statistics package tests.

I haven't checked any of the other source files.

yeah, the "+1"s were concerning - but it eventually just seemed completely wrong without them.  Alternative would be to compare (k < len-1) which perhaps is a more common construct to compare a 0-based index to the length of the array.

Orion Poplawski <opoplawski>
Mon 30 Jul 2018 07:55:31 PM UTC, comment #1: 

(io maintainer here)

Thanks.
Did you run the tests? csv2ell does have a few regression tests included.
(I'm a bit worried about the "+1" -'s you added, hopefully they doesn't screw up the proper indexing).

Are those offending construct also in cell2csv, csvexplode and csvconcat?

Maybe such constructs also exist in he mapping package in the gdalread.cc module.

Note: I'm by no means proficient in C / C++. I've adapted those modules quite a bit over the years but that sort of functional programming was no so hard :-)

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Sun 29 Jul 2018 11:32:03 PM UTC, original submission:  

Fedora is starting to build with -D_GLIBCXX_ASSERTIONS which is exposing some incorrect C++ string accesses.  The attached patch fixes some aborts in octave-io.

Orion Poplawski <opoplawski>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #44655:  octave-io-string.patch added by opoplawski (1KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by opoplawski (Submitted the item)
  •  

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

    Only project members can vote.

     

     

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-08-03 philipnienhuis StatusNone => Fixed
        Open/ClosedOpen => Closed
    2018-07-31 philipnienhuis Assigned toNone => philipnienhuis
        Operating SystemGNU/Linux => Any
        Summary(octave-forge[io] Fix string access in csv2ell => [octave-forge](io) Fix string access in csv2ell
    2018-07-30 philipnienhuis Summaryio: Fix string access => (octave-forge[io] Fix string access in csv2ell
    2018-07-29 opoplawski Attached File- => Added octave-io-string.patch, #44655

    Back to the top


    Powered by Savane 3.5