bugGNU Octave - Bugs: bug #55347, PVS studio static analysis report

 
 

bug #55347: PVS studio static analysis report

Submitter:  None
Submitted:  Mon 31 Dec 2018 09:42:06 AM UTC
   
 
Category:  None Severity:  4 - Important
Priority:  5 - Normal Item Group:  Other
Status:  Fixed Assigned to:  None
Originator Name:  Pavel Shlyak Originator Email:  -email is unavailable-
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

Fri 11 Jan 2019 09:34:46 PM UTC, comment #53: 

Just a note regarding my comment #30: It turned out that the use of the settings object in case of a non-accessible settings file does not lead to a crash (see bug #49736).

Torsten Lilge <ttl>
Group Member
Thu 10 Jan 2019 05:33:45 PM UTC, comment #52: 

Filed as  bug #55440

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Thu 10 Jan 2019 05:20:06 PM UTC, comment #51: 

Dmitri: Yes, I'd say open a second report about the clang static analyzer.  I took a quick look at some of the reports and it seems that it gets confused by the way we do reference counting for various classes.

John W. Eaton <jwe>
Group administrator
Thu 10 Jan 2019 04:29:05 AM UTC, comment #50: 

I run clang's static analyzer. The results
scan-build-2019-01-09-211448-26784-1.tar.xz (11 M)
should be available as



https://drive.google.com/file/d/1_RQNGwQ77rfI3cIwyYJFVNflrRYqGgnS/view


It is a directory that has an index.html file with links to the reports.

https://clang-analyzer.llvm.org/

Perhaps we should open a new bug report for this...

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Wed 09 Jan 2019 07:52:00 PM UTC, comment #49: 

Rik, your fix for pt-eval.cc to delete the lexer object created with new caused a double-free error because the parser takes ownership of the lexer and deletes it.  After your change, C-d at the prompt would cause a segfault.  I checked in the following changeset to fix:

http://hg.savannah.gnu.org/hgweb/octave/rev/4d6392c879d7

John W. Eaton <jwe>
Group administrator
Wed 09 Jan 2019 05:38:02 AM UTC, comment #48: 

The overwhelming majority of the issues identified have been examined or reviewed.  Marking as fixed and closing report.

Rik <rik5>
Group administrator
Mon 07 Jan 2019 08:48:47 PM UTC, comment #47: 

@jwe: Could you take a look at the really deep files like pt-eval.cc and oct-stream.cc?

This one looks like maybe two wrongs make a right


oct-stream.cc (893)
        V764         Possible incorrect order of arguments passed to 'process_conversion' function: 'type' and 'modifier'.


The function call at 893 for printf_format_list is


process_conversion (s, i, n, args, flags, fw, prec,
                    type, modifier);


for which the function prototype is


    void process_conversion (const std::string& s, size_t& i, size_t n,
                             int& args, std::string& flags, int& fw,
                             int& prec, char& modifier, char& type);


The static analyzer was pretty sophisticated to catch that since it couldn't rely on just type of argument as they are both 'char&'.  I think the reason why it may work is that the other process_conversion functions in the same file have the opposite ordering.  This is the scanf_format_list::process_conversion prototype.


    void process_conversion (const std::string& s, size_t& i, size_t n,
                             int& width, bool& discard, char& type,
                             char& modifier);


My guess is that the printf_format_list function body has been coded to expect "type", "modifier" ordering even thought the prototype is the other way around.

Rik <rik5>
Group administrator
Mon 07 Jan 2019 08:36:03 PM UTC, comment #46: 

@jwe: At the beginning of the static analyzer report it mentions that line numbers in lex.cc may be incorrect because of the presence of #line directives.


lex.cc (27)
        V011         Presence of #line directives may cause some diagnostic messages to have incorrect file name and line number.


I didn't think it this applied to lex.ll, but you're right that it must because I don't find any "yyg->yy_c_buf_p" variable in lex.ll.

You can probably just cross these off.

Rik <rik5>
Group administrator
Mon 07 Jan 2019 08:06:08 PM UTC, comment #45: 

@Torsten: Don't bother to cherry pick it.

Rik <rik5>
Group administrator
Mon 07 Jan 2019 08:01:39 PM UTC, comment #44: 

@Rik: I was going to cherry-pick Andy's V668 patch http://hg.savannah.gnu.org/hgweb/octave/rev/0249ba4c9589 into stable when I recognized that you have intentionally pushed it into the development branch (see comment #7). Cherry-picking it anyhow?

Torsten Lilge <ttl>
Group Member
Mon 07 Jan 2019 06:03:24 PM UTC, comment #43: 

Rik:

I'm not sure how the static analyzer reports line numbers, but if it is like a regular compiler, then it wouldn't even see lex.ll directly, but only lex.cc, which contains #line directives.  With that and all the macros involved in flex output, I think the line numbers for lex.ll can be confusing.

If the problem is the input line and column variables, why is it complaining about yyg->yy_c_buf_p?  In the generated code in lex.cc, yyg->yy_c_buf-p is definitely assigned twice successively because of the use of the macro YY_DO_BEFORE_ACTION inside the macro yyless.

It does look like the line number can be screwed up in the code you quoted in comment #40.


John W. Eaton <jwe>
Group administrator
Mon 07 Jan 2019 05:54:23 PM UTC, comment #42: 

A dynamic_cast will return nullptr for a valid pointer if you request a type that is different from the actual type (for example).  So if the rep is an idx_scalar_rep and you ask for idx_range_rep, it will fail.  But as you say, it is quite unlikely that this would go unnoticed.

I spent some time trying to figure out how to avoid the switch case and the dynamic casts, but couldn't see an easy way given the current design.  So maybe we could do better, but probably not without a major reworking of this set of classes.

John W. Eaton <jwe>
Group administrator
Mon 07 Jan 2019 05:32:08 PM UTC, comment #41: 

The static analyzer, perhaps legitamately, gets very suspicious of dynamic_casts.

Do we want to fix this?  I don't really know.  idx-vector is pretty fundamental and I'm not sure we want to introduce even a small delay in to one of the tightest inner loops of Octave.  It also seems likely that if we were getting this wrong at a structural level then we would have lots and lots of bug reports.  So as long as the input pointer before any dynamic_cast is valid I think it is an exceedingly rare event that it can't perform the dynamic_cast at runtime.

Rik <rik5>
Group administrator
Mon 07 Jan 2019 05:27:19 PM UTC, comment #40: 

@jwe: I crossed off all the bugs in lex.cc since that is an auto-generated file, but lex.ll is a source-controlled that we own.  Taking the first warning


lex.ll (391)
        V519         The 'yyg->yy_c_buf_p' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 391, 391.


I find the code


<COMMAND_START>(\.\.\.){ANY_EXCEPT_NL}*{NL} {
    curr_lexer->lexer_debug ("<COMMAND_START>(\\.\\.\\.){ANY_EXCEPT_NL}*{NL}");

    COMMAND_ARG_FINISH;

    curr_lexer->m_input_line_number++;
    curr_lexer->m_current_input_column = 1;

    HANDLE_STRING_CONTINUATION;
  }


The macro HANDLE_STRING_CONTINUATION is


#define HANDLE_STRING_CONTINUATION                      \
   do                                                   \
     {                                                  \
       curr_lexer->decrement_promptflag ();             \
       curr_lexer->m_input_line_number++;               \
       curr_lexer->m_current_input_column = 1;          \
                                                        \
       if (curr_lexer->is_push_lexer ())                \
         {                                              \
           if (curr_lexer->at_end_of_buffer ())         \
             return -1;                                 \
                                                        \
           if (curr_lexer->at_end_of_file ())           \
             return curr_lexer->handle_end_of_input (); \
         }                                              \
     }                                                  \
   while (0)


I think the static analyzer is suspicious because curr_lexer->m_input_line_number++ is being called twice in succession, as is curr_lexer->m_current_input_column = 1;

If this is all correct (which it probably is since the parser seems to work) then we can cross these off as false positives.




Rik <rik5>
Group administrator
Mon 07 Jan 2019 05:10:55 PM UTC, comment #39: 

As far as I can tell, the reports about successive assignments and a missing break statement in lex.ll are in code generated by flex.  So there's not much we can do about those.  It might not be the best, but I don't think the code is wrong.

John W. Eaton <jwe>
Group administrator
Mon 07 Jan 2019 04:59:22 PM UTC, comment #38: 

All the reports about idx-vector.h appear to be about the same thing.

The idx_vector functions index, assign, fill, loop, and bloop can't be virtual because they are templates.  So they use a switch statement to dispatch to the various types of idx_vector objects that we have and perform a dynamic_cast on the rep pointer.  If we somehow got the type wrong the dynamic_cast would fail and return nullptr.  I'm not sure what the best fix is here.  Use assert to check that the pointer is valid?

John W. Eaton <jwe>
Group administrator
Mon 07 Jan 2019 04:28:37 PM UTC, comment #37: 

@Andreas: Up to you whether you want to color the critical ones or not.

Rik <rik5>
Group administrator
Mon 07 Jan 2019 09:56:13 AM UTC, comment #36: 

should we color the items from the important/critical list red as it was previous the update?

Andreas Weber <andy1978>
Group Member
Sun 06 Jan 2019 10:19:41 PM UTC, comment #35: 

I'm fine with backporting or not.

The list of bugs at https://wiki.octave.org/PVS_static_analyzer_-_5.0_Release has been updated to reflect most of the things that were crossed of the original list.  It should be okay to look for new things on it to fix now.

Rik <rik5>
Group administrator
Sun 06 Jan 2019 08:55:18 PM UTC, comment #34: 

@Rik: I think you mean Andreas' patch http://hg.savannah.gnu.org/hgweb/octave/rev/0249ba4c9589. We could cherry-pick it it to stable.

Torsten Lilge <ttl>
Group Member
Sun 06 Jan 2019 08:50:10 PM UTC, comment #33: 

Okay, I had assumed that Torsten's patch was made on stable.  I will simply check them off the list for now.

Rik <rik5>
Group administrator
Sun 06 Jan 2019 08:23:30 PM UTC, comment #32: 

Cset 0249ba4c9589 seems to be applied to the default branch, but not to stable. I was asked to download and test the latter

Pavel Shlyak <emergency>
Sun 06 Jan 2019 08:20:24 PM UTC, comment #31: 

@rik5

> hg id
> aced09cc1721 (stable) tip

I didn't touch the sources since the analysis so this is exactly the same version

Pavel Shlyak <emergency>
Sun 06 Jan 2019 07:32:58 PM UTC, comment #30: 

@Rik: The creation of the single settings objects is tested for success. Thus, AFAIK, the pointer returned by get_settings always points to an existing and valid object. This settings object is not destroyed except for the gui's shutdown.

However, the settings file might become inaccessible. I am currently looking for a solution for this problem but since that will likely be a quite large changeset, it will not go into the stable branch.

To conclude, I suggest to ignore the warnings related to accessing the settings pointer without further testing against nullptr for now.

Torsten Lilge <ttl>
Group Member
Sun 06 Jan 2019 04:45:57 PM UTC, comment #29: 

@Pavel: Can you report with 'hg id' what version of the repository you used to prepare the second report?  I am seeing V668 warnings which were all fixed 4 days ago in cset 0249ba4c9589.

I'm worried that this latest report is invalid.

Rik <rik5>
Group administrator
Sun 06 Jan 2019 04:22:14 PM UTC, comment #28: 

I moved the old checklist to a new page on the wiki and have replaced it with the latest results still at https://wiki.octave.org/PVS_static_analyzer_-_5.0_Release.

I have marked off all of the libgnu and operator bugs.  It doesn't sound like we should invest too much time in fixing anything in the qterminal directory.

I still see a lot of warning relating to the settings variable in the GUI files.

Rik <rik5>
Group administrator
Sun 06 Jan 2019 03:39:09 PM UTC, comment #27: 

We have run clang-tidy in the past as a lint checker.  I have been much more impressed with the PVS results though.  Given that time is running out on the 5.0 release I think using only one tool will be enough for this release cycle.

Rik <rik5>
Group administrator
Sun 06 Jan 2019 03:06:21 PM UTC, comment #26: 

@mmuetzel
The naive way to check the platform dependent code is, obviously, to run the analysis on Windows :) However, I cannot do it myself as I have no Windows installed.
P. S. I would also recommend trying clang-tidy. It is sometimes not that accurate as PVS-studio, but still worth trying. Free and open source :)

Pavel Shlyak <emergency>
Sun 06 Jan 2019 02:23:25 PM UTC, comment #25: 

There also seems to be at least one bug in gnulib (Windows only) that is attributed to GNU Octave on PVS's website:
https://www.viva64.com/en/examples/v507/

Do we want to fix this downstream for us? I don't know the procedure to get this fixed upstream.

It'd be nice if we could check the platform dependent code (Windows), too.
How could this be done? With MXE Octave?

Markus Mützel <mmuetzel>
Group administrator
Sun 06 Jan 2019 09:37:56 AM UTC, comment #24: 

Regarding the use of qterminal: There were several changes in our fork that are tightly related to the use for octave. Therefore, it might be quite difficult to use the qterminal lib. Moreover, as already mentioned by Mike, the long term plan is to replace a terminal solution by another approach, which is less dependent on the target platform.

Torsten Lilge <ttl>
Group Member
Sun 06 Jan 2019 08:55:12 AM UTC, comment #23: 

Steps to reproduce the report:
- Download PVS studio (free for Open Source projects)
- ./bootstrap
- ./configure --disable-java
- pvs-studio-analyzer trace -- make -j2
- pvs-studio-analyzer analyze
- plog-converter -a GA:1,2,3 -t html -o pvs123.html ./PVS-Studio.log
- plog-converter -a GA:1,2 -t html -o pvs12.html ./PVS-Studio.log
- plog-converter -a GA:1 -t html -o pvs1.html ./PVS-Studio.log

Pavel Shlyak <emergency>
Sat 05 Jan 2019 07:57:07 PM UTC, comment #22: 

Oh, thank you, I was searching for qterminal, I see qtermwidget now.

Mike Miller <mtmiller>
Group Member
Sat 05 Jan 2019 07:48:01 PM UTC, comment #21: 

@mtmiller
Ubuntu:
sudo apt install libqtermwidget5-0
Fedora:
dnf install qtermwidget

Pavel Shlyak <emergency>
Sat 05 Jan 2019 07:43:40 PM UTC, comment #20: 

Pavel - is qterminal available as a library in any distribution? In Debian and Ubuntu it looks to me like it's only available as a program.

Elsewhere we've discussed removing qterminal entirely in favor of a text widget and avoiding trying to implement a terminal emulator entirely.

Mike Miller <mtmiller>
Group Member
Sat 05 Jan 2019 07:32:45 PM UTC, comment #19: 

@jwe for the qterminal - lxqt team maintains up to date version of the library. Many distribution repositories contain it. I wanna split this widget out of octave. I will open a ticket here later. Refer this issue for details: https://github.com/lxqt/qtermwidget/issues/240

Pavel Shlyak <emergency>
Sat 05 Jan 2019 06:53:01 PM UTC, comment #18: 

@rik5 will do it ASAP

Pavel Shlyak <emergency>
Sat 05 Jan 2019 06:45:09 PM UTC, comment #17: 

@shlyak: Could you pull the stable branch of Octave from the Mercurial repository and re-run the PVS static analysis tool?

Quite a bit has been fixed and it is becoming confusing at  https://wiki.octave.org/PVS_static_analyzer_-_5.0_Release to see what remains to be inspected.

Rik <rik5>
Group administrator
Fri 04 Jan 2019 08:50:23 PM UTC, comment #16: 

For the qterminal code, I think we can just fix things that look important.  I see just 3 items in the critical list.  There are 24 more in the full list, but I'd say skip them for now.

John W. Eaton <jwe>
Group administrator
Fri 04 Jan 2019 08:25:51 PM UTC, comment #15: 

@jwe: What is your opinion on fixing anything at all in the libgui/qterminal directory?  It's not really our code, on the other hand there could still be segfaults lurking there.

Rik <rik5>
Group administrator
Fri 04 Jan 2019 04:53:29 PM UTC, comment #14: 

I think we can go a bit further with the existing list, but then re-run it.  Right now, it is relatively easy to see what remains to be checked, but if we re-run the tool right now then all of the crossed off items like the gnulib files and "Odd precise comparison" warnings will return.  Then we will be in the position of scrubbing the list for an hour just to find which items are real.

The analysis has caught many, many more things than I had expected and I will post a note to the Octave-Maintainer's list about that.

Rik <rik5>
Group administrator
Fri 04 Jan 2019 03:01:38 PM UTC, comment #13: 

Now that we have fixed a few items, how difficult is it to run the tool again and get an updated list of problems?

John W. Eaton <jwe>
Group administrator
Thu 03 Jan 2019 07:53:23 PM UTC, comment #12: 

@John: get_settings always returns a pointer to an existing object. Therefore, checking the pointer is not necessary in my opinion. But in a case like that of bug #49736 (too many open files) the settings object does not work as expected; at least from my tests so far.

The plan is to read the state of the settings object in get_settings and to return a settings stub that just returns default values without accessing any files.

Torsten Lilge <ttl>
Group Member
Thu 03 Jan 2019 03:13:07 PM UTC, comment #11: 

@Torsten: I don't know how you are planning to fix the problem with the pointer to the settings object, but can we make the m_settings and m_default_settings members in the resource_manager class either values or references so that they can be guaranteed to exist and then the get_settings function can return a reference?  That way we could avoid the need for any checks for valid pointers.  Or is there some case in which it is valid for the settings object(s) to not exist?

John W. Eaton <jwe>
Group administrator
Thu 03 Jan 2019 05:27:52 AM UTC, comment #10: 

I filed a new bug report #55366 about the second argument to save_binary being a non-constant reference.  This is likely to be incorrect, but would involve too much change right now to be implemented on the stable branch.  I have crossed off the bugs pertaining to this on the PVS report.

Rik <rik5>
Group administrator
Wed 02 Jan 2019 09:35:27 PM UTC, comment #9: 

Extensive changes are best done on the development branch.

Rik <rik5>
Group administrator
Wed 02 Jan 2019 09:17:23 PM UTC, comment #8: 

I am currently trying to fix the numerous "V522: There might be dereferencing of a potential null pointer 'settings'" by adding some tests in resource_manager::get_settings () method. This should also prevent the GUI from crashing with signal 5 when the max number of files was opened (bug #49736) since the access to the settings file might fail even though "settings" points to a valid QSettings instance.

Since this cset will affect many files and methods I am wondering whether it should go into the default or stable branch.

Torsten Lilge <ttl>
Group Member
Wed 02 Jan 2019 06:00:11 PM UTC, comment #7: 

@Andreas: I pushed your changeset to the development branch here (https://hg.savannah.gnu.org/hgweb/octave/rev/0249ba4c9589).

I made one modification which was to include a fix for file-editor-tab.cc.  Can you go ahead and cross off V668 warnings from the list at https://wiki.octave.org/PVS_static_analyzer_-_5.0_Release

Rik <rik5>
Group administrator
Wed 02 Jan 2019 04:56:50 PM UTC, comment #6: 

Eliminating checks to ensure valid pointers after new seems fine, but I don't think it is necessary to do that on the stable branch.

John W. Eaton <jwe>
Group administrator
Wed 02 Jan 2019 01:18:22 PM UTC, comment #5: 

Here is a patch which removes the detected V668 (unnecessary pointer checks)

(file #45842)

Andreas Weber <andy1978>
Group Member
Mon 31 Dec 2018 08:35:34 PM UTC, comment #4: 

@Doug: I pushed your cset here https://hg.savannah.gnu.org/hgweb/octave/rev/13c62f51e830.  Only change I made was to the commit message to include the bug number (bug #55347).  This helps us quickly link a patch with the original problem.

I also crossed off the bug at https://wiki.octave.org/PVS_static_analyzer_-_5.0_Release.

Rik <rik5>
Group administrator
Mon 31 Dec 2018 07:58:01 PM UTC, comment #3: 

I added the full report to the octave wiki at https://wiki.octave.org/PVS_static_analyzer_-_5.0_Release.  You can mark off bugs that have been fixed by using the <strike> and </strike> formatting commands.

I'll push your changeset momentarily.

Rik <rik5>
Group administrator
Mon 31 Dec 2018 06:36:38 PM UTC, comment #2: 

I have a patch that fixes one of the items attached.

How are we going to keep track of what is fixed and what is just a false positive?


(file #45818)

Doug Stewart <dastew>
Mon 31 Dec 2018 04:51:00 PM UTC, comment #1: 

Thanks for doing this.  Will take a look prior to the 5.0 release to see what can easily be cleaned up.

Rik <rik5>
Group administrator
Mon 31 Dec 2018 09:42:06 AM UTC, original submission:  

Hello.
I have tried to run a commercial static analysis tool against the project. The short report (only critical messages) and full report are provided in attachments. Hope it helps to improve the product. Have a nice day!

Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #45879:  pvs1.html added by None (51KiB - text/html - 1 - most critical 2 - critical 3- all warnings)
file #45880:  pvs123.html added by None (876KiB - text/html - 1 - most critical 2 - critical 3- all warnings)
file #45881:  pvs12.html added by None (244KiB - text/html - 1 - most critical 2 - critical 3- all warnings)
file #45842:  V668_new_check.patch added by andy1978 (21KiB - text/x-patch)
file #45818:  26131.patch added by dastew (1KiB - text/x-patch)
file #45807:  full_report.html added by None (920KiB - text/html)
file #45808:  critical.html added by None (58KiB - text/html)
file #45809:  pvs.tasks added by None (116KiB - 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 dasergatskov (Posted a comment)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by emergency (Posted a comment)
  • -email is unavailable- added by ttl (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by andy1978 (Updated the item)
  • -email is unavailable- added by dastew (Updated the item)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by None (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 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-01-09 rik5 StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2019-01-06 None Attached File- Added pvs1.html, #45879
        Attached File- Added pvs123.html, #45880
        Attached File- Added pvs12.html, #45881
    2019-01-02 andy1978 Attached File- Added V668_new_check.patch, #45842
    2018-12-31 rik5 StatusNone In Progress
    2018-12-31 dastew Attached File- Added 26131.patch, #45818
    2018-12-31 rik5 Severity3 - Normal 4 - Important
        Item GroupNone Other
    2018-12-31 None Attached File- Added full_report.html, #45807
        Attached File- Added critical.html, #45808
        Attached File- Added pvs.tasks, #45809

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code