bugGNU Octave - Bugs: bug #63437, Null pointer dereference in...

 
 

bug #63437: Null pointer dereference in file-editor-tab.cc

Submitter:  Arun Giridhar <arungiridhar>
Submitted:  Mon 28 Nov 2022 10:05:41 PM UTC
   
 
Category:  GUI Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Segfault, Bus Error, etc.
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * stable Operating System:  * Any
Fixed Release:  8.1.0 Planned Release:  8.1.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 05 Feb 2023 03:01:24 PM UTC, comment #50: 

Afaict, the original issue (and probably also most of the OT issues) in this report has been fixed.

Closing report.

Markus Mützel <mmuetzel>
Group administrator
Fri 13 Jan 2023 05:41:14 AM UTC, comment #49: 

For slow printing of spchol image see newly filed bug #63646.

Rik <rik5>
Group administrator
Wed 04 Jan 2023 04:10:31 AM UTC, comment #48: 

It looks like the slowdown is due to sparseimages()
(this is running on dev tree)



octave:1> tic; sparseimages('/tmp', 'spchol', 'eps'); toc
Elapsed time is 68.8608 seconds.
octave:2> ver
----------------------------------------------------------------------
GNU Octave Version: 7.3.0 (hg id: 2c037ce00450)



octave:2> tic; sparseimages('/tmp', 'spchol', 'eps'); toc
Elapsed time is 92.428 seconds.
octave:3> ver
----------------------------------------------------------------------
GNU Octave Version: 9.0.0 (hg id: 4dad2c2c8dda)


The size of files is also different


2.5M -rw-r--r-- 1 2.5M Jan  3 22:53 spchol_73.eps
1.5M -rw-r--r-- 1 1.5M Jan  3 22:58 spchol_900.eps


though the pictures look the same.
Similarly with other sp* pics.
In any case that should be another bug report.

Dmitri.
--



Dmitri A. Sergatskov <dasergatskov>
Tue 03 Jan 2023 11:50:55 PM UTC, comment #47: 

Thanks, I pushed this change:

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

I've also seen delays when generating the .qhc file.  I'm not sure why.

John W. Eaton <jwe>
Group administrator
Tue 03 Jan 2023 11:39:21 PM UTC, comment #46: 

Yes with #include <array> it now compiles.
For some (other?) reason

GEN      doc/interpreter/octave_interpreter.qhc

took a very long time...

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Tue 03 Jan 2023 11:29:00 PM UTC, comment #45: 

i just tried with an different/older clang
AMD clang version 14.0.6 (CLANG: AOCC_4.0.0-Build#434 2022_10_28) (based on LLVM Mirror.Version.14.0.6)
(https://www.amd.com/en/developer/aocc.html#downloads)
and I got the same error (this is with #include <vector>":

/bin/sh ./libtool  --tag=CXX   --mode=compile clang++ -DHAVE_CONFIG_H -I. -I..   -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/harfbuzz -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -pthread  -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/harfbuzz -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -I/usr/include/libxml2  -DOCTGUI_DLL -I/usr/include/qt5/QtCore -I/usr/include/qt5 -I/usr/include/qt5/QtGui -I/usr/include/qt5/QtHelp -I/usr/include/qt5/QtWidgets -I/usr/include/qt5/QtSql -I/usr/include/qt5/QtNetwork -I/usr/include/qt5/QtPrintSupport -I/usr/include/qt5/QtXml -I../libgui/qterminal/libqterminal -Ilibgui/src -I../libgui/src -Ilibgui/graphics -I../libgui/graphics -I../libgui/src/m-editor -Iliboctave -I../liboctave/array -Iliboctave/numeric -I../liboctave/numeric -Iliboctave/operators -I../liboctave/operators -I../liboctave/system -I../liboctave/util -Ilibinterp -I../libinterp -Ilibinterp/parse-tree -I../libinterp/parse-tree -Ilibinterp/corefcn -I../libinterp/corefcn -I../libinterp/octave-value -I../liboctave/wrappers  -fPIC -pthread -fopenmp -Wall -W -Wshadow -Woverloaded-virtual -Wold-style-cast -Wformat -Wpointer-arith -Wwrite-strings -Wcast-align -Wcast-qual -fvisibility=hidden -O2 -mtune=native -mavx -mavx2 -flto -MT libgui/src/libgui_src_la-gui-settings.lo -MD -MP -MF libgui/src/.deps/libgui_src_la-gui-settings.Tpo -c -o libgui/src/libgui_src_la-gui-settings.lo `test -f 'libgui/src/gui-settings.cc' || echo '../'`libgui/src/gui-settings.cc
libtool: compile:  clang++ -DHAVE_CONFIG_H -I. -I.. -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/harfbuzz -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -pthread -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/harfbuzz -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -I/usr/include/libxml2 -DOCTGUI_DLL -I/usr/include/qt5/QtCore -I/usr/include/qt5 -I/usr/include/qt5/QtGui -I/usr/include/qt5/QtHelp -I/usr/include/qt5/QtWidgets -I/usr/include/qt5/QtSql -I/usr/include/qt5/QtNetwork -I/usr/include/qt5/QtPrintSupport -I/usr/include/qt5/QtXml -I../libgui/qterminal/libqterminal -Ilibgui/src -I../libgui/src -Ilibgui/graphics -I../libgui/graphics -I../libgui/src/m-editor -Iliboctave -I../liboctave/array -Iliboctave/numeric -I../liboctave/numeric -Iliboctave/operators -I../liboctave/operators -I../liboctave/system -I../liboctave/util -Ilibinterp -I../libinterp -Ilibinterp/parse-tree -I../libinterp/parse-tree -Ilibinterp/corefcn -I../libinterp/corefcn -I../libinterp/octave-value -I../liboctave/wrappers -fPIC -pthread -fopenmp -Wall -W -Wshadow -Woverloaded-virtual -Wold-style-cast -Wformat -Wpointer-arith -Wwrite-strings -Wcast-align -Wcast-qual -fvisibility=hidden -O2 -mtune=native -mavx -mavx2 -flto -MT libgui/src/libgui_src_la-gui-settings.lo -MD -MP -MF libgui/src/.deps/libgui_src_la-gui-settings.Tpo -c ../libgui/src/gui-settings.cc  -fPIC -DPIC -o libgui/src/.libs/libgui_src_la-gui-settings.o
../libgui/src/gui-settings.cc:542:46: error: implicit instantiation of undefined template 'std::array<std::basic_string<char>, 6>'
            const std::array<std::string, 6> env_vars =
                                             ^
/usr/lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/tuple:1595:45: note: template is declared here
  template<typename _Tp, size_t _Nm> struct array;
                                            ^
1 error generated.
make[2]: *** [Makefile:18300: libgui/src/libgui_src_la-gui-settings.lo] Error 1


Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Tue 03 Jan 2023 11:14:18 PM UTC, comment #44: 

Oh, first I see now that it is std::array, not vector and that it seems to have the declaration but not be able to instantiate it?  Weird, but the resource-manager.cc file did include <array> and that header is not explicitly included in gui-settings.cc.  So, does including <array> in gui-settings.cc help?  If not, what exactly could be missing that is preventing instantiation there?

John W. Eaton <jwe>
Group administrator
Tue 03 Jan 2023 10:55:49 PM UTC, comment #43: 

Does not seem to help. Same error.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Tue 03 Jan 2023 10:45:38 PM UTC, comment #42: 

The definition of that variable was present in resource-manager.cc.  The function that contains it was just moved from the resource_manager class to the gui_settings class.

Does it help to add #include <vector> in the gui-settings.cc file?  Maybe GCC includes it implicitly in some other header file.

John W. Eaton <jwe>
Group administrator
Tue 03 Jan 2023 10:17:03 PM UTC, comment #41: 

OK, gcc error got resolved by doing fresh clone etc..., but clang error
seems to persist. This is on freshly cloned tree:


bin/sh ./libtool  --tag=CXX   --mode=compile clang++ -DHAVE_CONFIG_H -I. -I..   -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/harfbuzz -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -pthread  -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/harfbuzz -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -I/usr/include/libxml2  -DOCTGUI_DLL -I/usr/include/qt5/QtCore -I/usr/include/qt5 -I/usr/include/qt5/QtGui -I/usr/include/qt5/QtHelp -I/usr/include/qt5/QtWidgets -I/usr/include/qt5/QtSql -I/usr/include/qt5/QtNetwork -I/usr/include/qt5/QtPrintSupport -I/usr/include/qt5/QtXml -I../libgui/qterminal/libqterminal -Ilibgui/src -I../libgui/src -Ilibgui/graphics -I../libgui/graphics -I../libgui/src/m-editor -Iliboctave -I../liboctave/array -Iliboctave/numeric -I../liboctave/numeric -Iliboctave/operators -I../liboctave/operators -I../liboctave/system -I../liboctave/util -Ilibinterp -I../libinterp -Ilibinterp/parse-tree -I../libinterp/parse-tree -Ilibinterp/corefcn -I../libinterp/corefcn -I../libinterp/octave-value -I../liboctave/wrappers  -fPIC -pthread -fopenmp -Wall -W -Wshadow -Woverloaded-virtual -Wold-style-cast -Wformat -Wpointer-arith -Wwrite-strings -Wcast-align -Wcast-qual -fvisibility=hidden -O2 -mtune=native -mavx -mavx2 -flto=auto -MT libgui/src/libgui_src_la-gui-settings.lo -MD -MP -MF libgui/src/.deps/libgui_src_la-gui-settings.Tpo -c -o libgui/src/libgui_src_la-gui-settings.lo `test -f 'libgui/src/gui-settings.cc' || echo '../'`libgui/src/gui-settings.cc
libtool: compile:  clang++ -DHAVE_CONFIG_H -I. -I.. -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/harfbuzz -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -pthread -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/harfbuzz -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -I/usr/include/libxml2 -DOCTGUI_DLL -I/usr/include/qt5/QtCore -I/usr/include/qt5 -I/usr/include/qt5/QtGui -I/usr/include/qt5/QtHelp -I/usr/include/qt5/QtWidgets -I/usr/include/qt5/QtSql -I/usr/include/qt5/QtNetwork -I/usr/include/qt5/QtPrintSupport -I/usr/include/qt5/QtXml -I../libgui/qterminal/libqterminal -Ilibgui/src -I../libgui/src -Ilibgui/graphics -I../libgui/graphics -I../libgui/src/m-editor -Iliboctave -I../liboctave/array -Iliboctave/numeric -I../liboctave/numeric -Iliboctave/operators -I../liboctave/operators -I../liboctave/system -I../liboctave/util -Ilibinterp -I../libinterp -Ilibinterp/parse-tree -I../libinterp/parse-tree -Ilibinterp/corefcn -I../libinterp/corefcn -I../libinterp/octave-value -I../liboctave/wrappers -fPIC -pthread -fopenmp -Wall -W -Wshadow -Woverloaded-virtual -Wold-style-cast -Wformat -Wpointer-arith -Wwrite-strings -Wcast-align -Wcast-qual -fvisibility=hidden -O2 -mtune=native -mavx -mavx2 -flto=auto -MT libgui/src/libgui_src_la-gui-settings.lo -MD -MP -MF libgui/src/.deps/libgui_src_la-gui-settings.Tpo -c ../libgui/src/gui-settings.cc  -fPIC -DPIC -o libgui/src/.libs/libgui_src_la-gui-settings.o
../libgui/src/gui-settings.cc:541:46: error: implicit instantiation of undefined template 'std::array<std::basic_string<char>, 6>'
            const std::array<std::string, 6> env_vars =
                                             ^
/usr/bin/../lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/tuple:1595:45: note: template is declared here
  template<typename _Tp, size_t _Nm> struct array;
                                            ^
1 error generated.
make[2]: *** [Makefile:18300: libgui/src/libgui_src_la-gui-settings.lo] Error 1


Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Tue 03 Jan 2023 09:54:10 PM UTC, comment #40: 

Problems on Fedora (both clang 15 and gcc
clang version 15.0.6 (Fedora 15.0.6-3.fc37):


/bin/sh ./libtool  --tag=CXX   --mode=compile clang++ -DHAVE_CONFIG_H -I. -I..   -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/harfbuzz -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -pthread  -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/harfbuzz -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -I/usr/include/libxml2  -DOCTGUI_DLL -I/usr/include/qt5/QtCore -I/usr/include/qt5 -I/usr/include/qt5/QtGui -I/usr/include/qt5/QtHelp -I/usr/include/qt5/QtWidgets -I/usr/include/qt5/QtSql -I/usr/include/qt5/QtNetwork -I/usr/include/qt5/QtPrintSupport -I/usr/include/qt5/QtXml -I../libgui/qterminal/libqterminal -Ilibgui/src -I../libgui/src -Ilibgui/graphics -I../libgui/graphics -I../libgui/src/m-editor -Iliboctave -I../liboctave/array -Iliboctave/numeric -I../liboctave/numeric -Iliboctave/operators -I../liboctave/operators -I../liboctave/system -I../liboctave/util -Ilibinterp -I../libinterp -Ilibinterp/parse-tree -I../libinterp/parse-tree -Ilibinterp/corefcn -I../libinterp/corefcn -I../libinterp/octave-value -I../liboctave/wrappers  -fPIC -pthread -fopenmp -Wall -W -Wshadow -Woverloaded-virtual -Wold-style-cast -Wformat -Wpointer-arith -Wwrite-strings -Wcast-align -Wcast-qual -fvisibility=hidden -O2 -mtune=native -mavx -mavx2 -MT libgui/src/libgui_src_la-gui-settings.lo -MD -MP -MF libgui/src/.deps/libgui_src_la-gui-settings.Tpo -c -o libgui/src/libgui_src_la-gui-settings.lo `test -f 'libgui/src/gui-settings.cc' || echo '../'`libgui/src/gui-settings.cc
libtool: compile:  clang++ -DHAVE_CONFIG_H -I. -I.. -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/harfbuzz -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -pthread -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/harfbuzz -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -I/usr/include/libxml2 -DOCTGUI_DLL -I/usr/include/qt5/QtCore -I/usr/include/qt5 -I/usr/include/qt5/QtGui -I/usr/include/qt5/QtHelp -I/usr/include/qt5/QtWidgets -I/usr/include/qt5/QtSql -I/usr/include/qt5/QtNetwork -I/usr/include/qt5/QtPrintSupport -I/usr/include/qt5/QtXml -I../libgui/qterminal/libqterminal -Ilibgui/src -I../libgui/src -Ilibgui/graphics -I../libgui/graphics -I../libgui/src/m-editor -Iliboctave -I../liboctave/array -Iliboctave/numeric -I../liboctave/numeric -Iliboctave/operators -I../liboctave/operators -I../liboctave/system -I../liboctave/util -Ilibinterp -I../libinterp -Ilibinterp/parse-tree -I../libinterp/parse-tree -Ilibinterp/corefcn -I../libinterp/corefcn -I../libinterp/octave-value -I../liboctave/wrappers -fPIC -pthread -fopenmp -Wall -W -Wshadow -Woverloaded-virtual -Wold-style-cast -Wformat -Wpointer-arith -Wwrite-strings -Wcast-align -Wcast-qual -fvisibility=hidden -O2 -mtune=native -mavx -mavx2 -MT libgui/src/libgui_src_la-gui-settings.lo -MD -MP -MF libgui/src/.deps/libgui_src_la-gui-settings.Tpo -c ../libgui/src/gui-settings.cc  -fPIC -DPIC -o libgui/src/.libs/libgui_src_la-gui-settings.o
../libgui/src/gui-settings.cc:541:46: error: implicit instantiation of undefined template 'std::array<std::basic_string<char>, 6>'
            const std::array<std::string, 6> env_vars =
                                             ^
/usr/bin/../lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/tuple:1595:45: note: template is declared here
  template<typename _Tp, size_t _Nm> struct array;
                                            ^
1 error generated.
make[2]: *** [Makefile:18300: libgui/src/libgui_src_la-gui-settings.lo] Error 1

gcc version 12.2.1 20221121 (Red Hat 12.2.1-4) (GCC):


libtool: compile:  g++ -DHAVE_CONFIG_H -I. -I.. -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/harfbuzz -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -pthread -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/harfbuzz -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -I/usr/include/libxml2 -DOCTGUI_DLL -I/usr/include/qt5/QtCore -I/usr/include/qt5 -I/usr/include/qt5/QtGui -I/usr/include/qt5/QtHelp -I/usr/include/qt5/QtWidgets -I/usr/include/qt5/QtSql -I/usr/include/qt5/QtNetwork -I/usr/include/qt5/QtPrintSupport -I/usr/include/qt5/QtXml -I../libgui/qterminal/libqterminal -Ilibgui/src -I../libgui/src -Ilibgui/graphics -I../libgui/graphics -I../libgui/src/m-editor -Iliboctave -I../liboctave/array -Iliboctave/numeric -I../liboctave/numeric -Iliboctave/operators -I../liboctave/operators -I../liboctave/system -I../liboctave/util -Ilibinterp -I../libinterp -Ilibinterp/parse-tree -I../libinterp/parse-tree -Ilibinterp/corefcn -I../libinterp/corefcn -I../libinterp/octave-value -I../liboctave/wrappers -fPIC -pthread -fopenmp -Wall -W -Wshadow -Woverloaded-virtual -Wold-style-cast -Wformat -Wpointer-arith -Wwrite-strings -Wcast-align -Wcast-qual -fvisibility=hidden -O2 -mtune=native -mavx -mavx2 -flto=auto -MT libgui/src/m-editor/libgui_src_la-file-editor.lo -MD -MP -MF libgui/src/m-editor/.deps/libgui_src_la-file-editor.Tpo -c ../libgui/src/m-editor/file-editor.cc  -fPIC -DPIC -o libgui/src/m-editor/.libs/libgui_src_la-file-editor.o
../libgui/src/m-editor/file-editor.cc: In member function 'virtual void octave::file_editor::request_open_file(const QString&, const QString&, int, bool, bool, bool, const QString&, int, const QString&)':
../libgui/src/m-editor/file-editor.cc:1638:23: error: 'reusing' was not declared in this scope
 1638 |                 if (! reusing)
      |                       ^~~~~~~
make[2]: *** [Makefile:18335: libgui/src/m-editor/libgui_src_la-file-editor.lo] Error 1


Dmitri.
--


Dmitri A. Sergatskov <dasergatskov>
Tue 03 Jan 2023 08:34:10 PM UTC, comment #39: 

Speaking for myself as the original submitter, it's OK to close it. The original static analyzer warning is fully resolved.

Arun Giridhar <arungiridhar>
Group Member
Tue 03 Jan 2023 07:39:34 PM UTC, comment #38: 
John W. Eaton <jwe>
Group administrator
Tue 20 Dec 2022 12:37:28 PM UTC, comment #37: 

Is this bug fixed after jwe's check-ins on December 4th?

Also, is there any relation to newly reported bug #63548 where there is a segfault originating in file-editor-tab.cc?

Rik <rik5>
Group administrator
Wed 07 Dec 2022 08:32:20 AM UTC, comment #36: 

Re comment #30:
I installed Cygwin locally. The symbol that is exported by the Qt5Core library is the following:

$  nm -C /lib/libQt5Core.dll.a | grep QFile::open
0000000000000000 T QFile::open(int, QFlags<QIODevice::OpenModeFlag>, QFlags<QFileDevice::FileHandleFlag>)
0000000000000000 T QFile::open(__sFILE64*, QFlags<QIODevice::OpenModeFlag>, QFlags<QFileDevice::FileHandleFlag>)
0000000000000000 T QFile::open(QFlags<QIODevice::OpenModeFlag>)


Note the `__sFILE64*` instead of the `__sFILE*` that we'd like to use.
Is there maybe some header file that was included earlier but now no longer is included that could have switched between 64-bit and 32-bit file pointers?

FWIW, the declaration of that function in the Qt5 headers installed by Cygwin:

    bool open(FILE *f, OpenMode ioFlags, FileHandleFlags handleFlags=DontCloseHandle);


Markus Mützel <mmuetzel>
Group administrator
Tue 06 Dec 2022 04:33:12 PM UTC, comment #35: 

I believe the icon problem is fixed with this changeset:

http://hg.savannah.gnu.org/hgweb/octave/rev/ab33554f97d2

John W. Eaton <jwe>
Group administrator
Mon 05 Dec 2022 09:15:17 PM UTC, comment #34: 

Instead of displaying the wizard when the file doesn't exist, it displays it unless the new "global_skip_welcome_wizard" setting is true.  (Maybe I should drop the "global_" prefix from the key name?)  Anyway, if it is not present, it defaults to false.  Old settings files won't have it, so you will see the welcome wizard one extra time when upgrading.  I don't see the harm, but we could also try harder to avoid that if it seems important.

I'm not sure what I did to break the icons.  I see the issue now, but seemed to be working for me when I was making that part of my changes.

John W. Eaton <jwe>
Group administrator
Mon 05 Dec 2022 08:56:27 PM UTC, comment #33: 

First observations after the changes regarding the settings:

  • The new default build started with the splash screen, which only appears when running for the first time, although the settings file was existing. The subsequent starts were normal.
  • The gui seems to ignore the selected icon set.


I will try to find the issues and make more tests the next days.

Torsten Lilge <ttl>
Group Member
Mon 05 Dec 2022 04:18:35 PM UTC, comment #32: 

If I read the logs correctly, the commands for linking the octave-svgconvert executable were:

/bin/sh ./libtool  --tag=CXX   --mode=link g++  -pthread -fopenmp -Wall -W -Wshadow -Woverloaded-virtual -Wold-style-cast -Wformat -Wpointer-arith -Wwrite-strings -Wcast-align -Wcast-qual  -g -O2    -o src/octave-svgconvert.exe src/octave_svgconvert-octave-svgconvert.o -lQt5Xml -lQt5Core -lQt5Sql -lQt5Help -lQt5Network -lQt5PrintSupport -lQt5Gui -lQt5Widgets  -lqscintilla2-qt5 -lpthread -lm


Cygwin is an "intermediate" between Windows and Linux.

It looks like we don't need to do anything special when linking for MinGW (where linking suceeds):

/bin/sh ./libtool  --tag=CXX   --mode=link g++  -pthread -fopenmp -Wall -W -Wshadow -Woverloaded-virtual -Wold-style-cast -Wformat -Wpointer-arith -Wwrite-strings -Wcast-align -Wcast-qual  -g -O2  -municode  -o src/octave-svgconvert.exe src/octave_svgconvert-octave-svgconvert.o -lQt5Help -lQt5Sql -lQt5Network -lQt5PrintSupport -lQt5Widgets -lQt5Gui -lQt5Xml -lQt5Core -lqscintilla2_qt5 -lshlwapi -lgdi32 -lpthread -lm


One thing that might be a significant difference: Tho order in which the Qt libraries are linked differs for the two targets...

Markus Mützel <mmuetzel>
Group administrator
Mon 05 Dec 2022 03:47:17 PM UTC, comment #31: 

RE: Comment #29, http://hg.savannah.gnu.org/hgweb/octave/rev/e518e1f7e944 should fix it.

I don't know what the problem is with the other error shown in comment #30.  Do you have the complete command used for linking the octave-svgconvert executable?  The following is just a guess, but from digging around a bit, octave-svgconvert.cc has this code:


      // Read from stdin
      if (! file.open (stdin, QIODevice::ReadOnly | QIODevice::Text))
        {


and the docs for QFile::open (https://doc.qt.io/qt-5/qfile.html#open-2) say:

Note for the Windows Platform

[...]

You need to enable support for console applications in order to use the stdin, stdout and stderr streams at the console. To do this, add the following declaration to your application's project file:


CONFIG += console


and it appears that for GCC, that CMake config option will set the linker flag "-Wl,-subsystem,console".  For MSVC, it is "/SUBSYSTEM:CONSOLE"

John W. Eaton <jwe>
Group administrator
Mon 05 Dec 2022 09:08:48 AM UTC, comment #30: 

Not sure if this is also related. But linking svgconvert for Cygwin also started to fail:
https://github.com/gnu-octave/octave/actions/runs/3616932076/jobs/6095331859#step:10:29241

/usr/lib/gcc/x86_64-pc-cygwin/11/../../../../x86_64-pc-cygwin/bin/ld: src/octave_svgconvert-octave-svgconvert.o: in function `main':
/cygdrive/d/a/octave/octave/.build/../src/octave-svgconvert.cc:912: undefined reference to `__imp__ZN5QFile4openEP7__sFILE6QFlagsIN9QIODevice12OpenModeFlagEES2_IN11QFileDevice14FileHandleFlagEE'
collect2: error: ld returned 1 exit status


The undefined symbol demangles to
QFile::open(__sFILE*, QFlags<QIODevice::OpenModeFlag>, QFlags<QFileDevice::FileHandleFlag>)

Markus Mützel <mmuetzel>
Group administrator
Mon 05 Dec 2022 08:43:02 AM UTC, comment #29: 

Building on macOS fails since the recent changes:
https://github.com/gnu-octave/octave/actions/runs/3616932076/jobs/6095332383

In file included from ../libgui/qterminal/libqterminal/QTerminal.cc:38:
../libgui/src/gui-preferences-global.h:143:1: error: no matching constructor for initialization of 'const gui_pref'
global_use_native_dialogs ("use_native_file_dialogs", QVariant (false), true);
^                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../libgui/src/gui-preferences.h:39:3: note: candidate constructor not viable: requires 2 arguments, but 3 were provided
  gui_pref (const QString& key_arg, const QVariant& def_arg)
  ^
../libgui/src/gui-preferences.h:45:3: note: candidate constructor not viable: requires 1 argument, but 3 were provided
  gui_pref (const gui_pref&) = delete;
  ^
1 error generated.


Markus Mützel <mmuetzel>
Group administrator
Mon 05 Dec 2022 04:47:56 AM UTC, comment #28: 

In addition to a few small fixes to the set of changes listed in comment #27, I've now pushed

http://hg.savannah.gnu.org/hgweb/octave/rev/34c3cd39c4b9
http://hg.savannah.gnu.org/hgweb/octave/rev/474e184321d3
http://hg.savannah.gnu.org/hgweb/octave/rev/ca7d58406f82

Now most of what remains is moving shortcut settings to the gui_settings (aka QSettings) object and eliminating the need to extract the shortcut_manager object from the base octave_qobject object.  Then nearly all of the direct uses of octave_qobject in the GUI widgets can be removed.  Then except for main_window, I think all the rest can be removed with just a little more refactoring.

John W. Eaton <jwe>
Group administrator
Sat 03 Dec 2022 04:14:50 AM UTC, comment #27: 

I pushed a series of changes to use local gui_settings objects and to access icons from the gui_settings class:

http://hg.savannah.gnu.org/hgweb/octave/rev/351a4c8e92c3
http://hg.savannah.gnu.org/hgweb/octave/rev/c6683fcc5c33
http://hg.savannah.gnu.org/hgweb/octave/rev/59c9da7c43d6
http://hg.savannah.gnu.org/hgweb/octave/rev/40b151abbb9b
http://hg.savannah.gnu.org/hgweb/octave/rev/726d2628752c
http://hg.savannah.gnu.org/hgweb/octave/rev/1f57ea5dfd4a
http://hg.savannah.gnu.org/hgweb/octave/rev/cd833a9baaa7
http://hg.savannah.gnu.org/hgweb/octave/rev/ad014fc78bd6
http://hg.savannah.gnu.org/hgweb/octave/rev/6dfaaf8ecf3b
http://hg.savannah.gnu.org/hgweb/octave/rev/0645ea65ca6b

There are still more changes to make to handle shortcuts in the gui_settings class and to eliminate references to the base_qobject object where possible.  I hope to be able to work on those changes within the next week.

I'm also marking this report as in progress, even though I think the issue is fixed for the 8.1.0 release.  I'd like to leave this open until I've pushed all my pending changes.  Thanks.

John W. Eaton <jwe>
Group administrator
Thu 01 Dec 2022 08:39:58 PM UTC, comment #26: 

Perfect. That makes it easier all round.

Arun Giridhar <arungiridhar>
Group Member
Thu 01 Dec 2022 08:33:46 PM UTC, comment #25: 

"It's OK to change `settings` to a `QSettings` object over time"

The gui_settings object is already derived from QSettings.  It adds no data, just some additional functions.  The resource_manager stores a single gui_settings object for use by Octave and provides a pointer to access it.  So we won't be changing the object that's ultimately used to store the settings.  We're just changing the way it is used.

For now, there's no need to remove checks to ensure that settings is a valid pointer.  That's wasted effort since I'll be completely removing the use of a pointer soon.  So don't bother.  My later edits will remove the checks anyway.

John W. Eaton <jwe>
Group administrator
Thu 01 Dec 2022 08:22:09 PM UTC, comment #24: 

Re comment #22, yes, agree fully. It's OK to change `settings` to a `QSettings` object over time, and it's not constrained by the 8.1 release. Hopefully that change will also reduce the number of race conditions being reported in a TSAN build.

Are there any upcoming edits to that pattern in file-editor-tab.cc and/or the other places with that pattern? (Changing `if (settings && settings->foo ())` to plain `if (settings->foo ())` for example).

Arun Giridhar <arungiridhar>
Group Member
Thu 01 Dec 2022 08:16:31 PM UTC, comment #23: 

I should have also mentioned that I intend to make additional changes to the way gui_settings is used on default, not stable.

John W. Eaton <jwe>
Group administrator
Thu 01 Dec 2022 08:06:18 PM UTC, comment #22: 

Back to the original issue reported here...

Do we want to try to avoid the static analyzer warning for something that is not likely important?  There are many other places where the pointer to the settings object is not checked before use.  I'd rather not add a bunch of checks for that when the settings object is always supposed to be valid.  If anything, we could add a check inside the resource_manager::get_settings function that pops up a critical warning/error in a message box that says something like "Something has gone horribly wrong and Octave has lost its settings!  This is not supposed to happen!"
and then attempt to close the application.  Or we could have it try to throw an exception and issue a message in the command window (that's what will happen with error_unless/error_if).

For now, I checked in the following change:

http://hg.savannah.gnu.org/hgweb/octave/rev/132547cc1ab2

If the new error that it introduces happens, then we can try to figure out how and why and work on a fix for a real problem.

Also, as discussed in the comments here, I intend to eliminate the pointer to a single settings object by using local gui_settings (AKA QSettings) objects that work the way Qt intended.  Then the local objects will always exist, won't be pointers, and we won't have to check that they are not null.

The QSettings object manages thread-safe storage to the underlying global settings data.  When the Qt docs say that operations on QSettings are not thread safe, it means that you can't create a QSettings object in one thread and use it in another without some locking.  But each QSettings object does perform locking when reading or writing the underlying global data.  So creating two distinct QSettings objects in different threads and calling value or setValue is OK.  So I think it just makes a lot more sense to use local QSettings objects instead of a single global one that then requires additional care to avoid possible threading issues.

John W. Eaton <jwe>
Group administrator
Wed 30 Nov 2022 09:16:21 PM UTC, comment #21: 

QSettings ultimately still provides access to a singleton object.  I don't think I understood that previously.  But now that I do, it seems better to just create local QSettings objects where they are needed to manage the access to the underlying singleton.  Uncoupling base_qobject from the widgets seems better to me now that I understand how QSettings is supposed to work.

John W. Eaton <jwe>
Group administrator
Wed 30 Nov 2022 08:23:13 PM UTC, comment #20: 

I do not see a real disadvantage in passing the base_qobject around compared to the way we previously accessed the settings, namely by calling the resource_manager singleton each time before  accessing the settings.

What would be the advantage of the system wide settings file? If the user's file is not present, the settings objects just selects the default and creates the settings file. But maybe I am missing something here.

And finally, yes, as we already have a derived settings class, I am (currently) seeing no reason why we should not be able to move the shortcut and resource manager methods into the settings.

Torsten Lilge <ttl>
Group Member
Wed 30 Nov 2022 07:59:43 PM UTC, comment #19: 

Our resource_manager and shortcut_objects were also originally singleton objects before I changed them to be owned by base_qobject.  But passing an instance of that object around to many of the GUI objects is probably not the best thing to do (again, my mistake).  So maybe it would be better to use something similar to QSettings for that.  Is there any reason we can't just use QSettings and a single init file for managing icons and shortcuts in addition to our other settings?

John W. Eaton <jwe>
Group administrator
Wed 30 Nov 2022 07:59:29 PM UTC, comment #18: 

RE: Comments 15 and 16:

Yes, I've been experimenting with using local instances of our gui_settings class (QSettings with some extra functions) and there are some initialization problemss that need to be sorted out.  But otherwise I think it should be possible to make it work and I now see that it is better to use local QSettings objects than to try to manage our own single settings object and pass around pointers to our base_qobject object in many classes.  Doing that was my mistake and I'd like to fix it.

RE: Comment 17:

Since QSettings already has a fallback mechanism, maybe it would be better to store all default settings in a system-wide settings file and then have the user's local settings file override those defaults?  The difficulty with this is creating and installing the platform-specific settings files.

For those not familiar with QSettings, it is an object that allows access to a singleton object.  Multiple QSettings objects may access a common internal collection of settings.

For reference: https://doc.qt.io/qt-5/qsettings.html

John W. Eaton <jwe>
Group administrator
Wed 30 Nov 2022 07:15:03 PM UTC, comment #17: 

In reply to comment #8: The default preferences are all defined in the gui-preferences-xx.h as they are used as defaults when the key is not (yet) found in the settings file. It would be no problem to just extract the defaults in case of a missing settings object.

Torsten Lilge <ttl>
Group Member
Wed 30 Nov 2022 07:01:57 PM UTC, comment #16: 

In reply to comment #9: While fixing some translation issues, I realized that workspace_model is created at the beginning of the base_qobject constructor and therefore well before defining the settings object is defined. However, the notice_settings method in workspace_model is not called from its constructor and should not be called before the main window is requesting all dock widgets to load their settings.

Torsten Lilge <ttl>
Group Member
Wed 30 Nov 2022 06:54:08 PM UTC, comment #15: 

I am not sure whether the default QSettings constructor really works if QSettings is used as a specific INI file without defining organization name, domain and application name with QCoreApplication. As far as I remember, this was the first use of QSettings for the GUI.

Torsten Lilge <ttl>
Group Member
Tue 29 Nov 2022 07:14:41 PM UTC, comment #14: 

Looking more at the QSettings class it seems that we been using it incorrectly.  Since QSettings itself is supposed to manage access to the global application settings we shouldn't have to maintain our own global QSettings object.  We should just be creating settings objects locally when needed and each one of those local QSettings objects accesses the global application settings.

For example: http://katecpp.github.io/qsettings/

Trying to have a single global QSettings object causes trouble and is not necessary.  I'm guessing that our attempt to do that was due to misunderstanding how QSettings works and how it is supposed to be used.

John W. Eaton <jwe>
Group administrator
Tue 29 Nov 2022 04:54:58 PM UTC, comment #13: 

If you have Clang, you can replicate the static analysis with `scan-build make` in a fresh build directory instead of just `make`.

Arun Giridhar <arungiridhar>
Group Member
Tue 29 Nov 2022 04:48:08 PM UTC, comment #12: 

I don't know how disruptive those fixes would be. But if they are pretty straight forward, I guess we could still aim for fixing them before the release.
In the past, it often looked like "the pressure was off" after a release and not much happened in this regard until shortly before the next release...

I'd just vote against adding new features now. But fixing those issues is always fine imho.

Markus Mützel <mmuetzel>
Group administrator
Tue 29 Nov 2022 04:31:54 PM UTC, comment #11: 

I am sure that there are a lot of threading issues to fix in the GUI.  Do we want to try to fix (some of) them

  • on the stable branch now
  • on the stable branch after the release
  • on default


?

John W. Eaton <jwe>
Group administrator
Tue 29 Nov 2022 04:30:02 PM UTC, comment #10: 

If we can ensure that the settings object is always valid, then I agree that we shouldn't need to have "if (settings)".

A separate issue is that the QSettings class that our gui_settings class is derived from is not thread safe, so simply calling notice_settings is not safe because we don't know what other threads might be using the gui_settings object at the same time.  Maybe I'm missing something, but I think all uses of the settings object should use signal/slot connections, not direct calls.

John W. Eaton <jwe>
Group administrator
Tue 29 Nov 2022 03:58:33 PM UTC, comment #9: 

The settings object is created early on in the constructor of `octave::main_window` before any of the widgets (including the editor widget) are created. I can't see how it can ever be a nullptr at that point.

Could we just remove the `if (settings)` and execute `notice_settings (settings, true);` unconditionally to silence the static analyzer?

Markus Mützel <mmuetzel>
Group administrator
Tue 29 Nov 2022 02:58:33 PM UTC, comment #8: 

I don't think we should have hard-coded defaults that are scattered across many files.  That would make changing defaults difficult and likely to lead to inconsistencies.

Instead, we should ensure that the settings object is initialized and used correctly.  If it is not valid for some reason, that should be a fatal error, preferably displayed in a way that will help us diagnose what the problem is.

Is it possible that the settings object is not defined when it is accessed?

Are operations on the settings object thread safe?  If not, then I'm not sure that we can expect things like


  gui_settings *settings = rmgr.get_settings ();
  if (settings)
    notice_settings (settings, true);


to work reliably because another thread may also be using the settings object.  Instead, we'll need to use a signal/slot connection to perform this operation.

John W. Eaton <jwe>
Group administrator
Tue 29 Nov 2022 12:31:17 PM UTC, comment #7: 

Re: comment #1, "I'm wondering whether it is possible that this code (or something similar) could be the reason that people report that Octave no longer starts for them?"

Are you referring to the cases where deleting octave-gui.ini fixes the problem?  Several of those users have reported that after deleting or renaming octave-gui.ini, octave successfully starts, and continues to start even if they copy back the old, supposedly corrupt, octave-gui.ini. If the issue was a file tab issue in the editor, I would think it would persist in such a case and recreate the problem. Unless restoring the default octave-gui.ini caused some other change outside of that ini file...

Nicholas Jankowski <nrjank>
Group Member
Tue 29 Nov 2022 12:26:20 PM UTC, comment #6: 

Your patch in comment #5 looks good and comprehensive to me.

Re comment #3 and comment #4, what graphics settings are loaded if `settings` is null? That's what I had been referring to as default settings to be passed to `notice_settings`. The question I didn't know was whether `settings` being null should stop execution and cause an error, or whether Octave could fill in default values. From your patch in comment #5 it looks like we can indeed fill in default values.

Arun Giridhar <arungiridhar>
Group Member
Tue 29 Nov 2022 08:32:25 AM UTC, comment #5: 

I'm attaching a patch that checks if the pointer to the settings object is valid in more places. If it isn't, it uses (hard-coded) defaults.

I don't know if the settings pointer can be a nullptr in all of those places. But adding the check probably doesn't hurt much either.


(file #54035)

Markus Mützel <mmuetzel>
Group administrator
Tue 29 Nov 2022 07:57:40 AM UTC, comment #4: 

I don't understand your proposal. If `settings` is a nullptr, there is no settings that could be noticed with anything...

Markus Mützel <mmuetzel>
Group administrator
Tue 29 Nov 2022 07:55:20 AM UTC, comment #3: 

Default values for the encoding would be better in that context.

What about `notice_settings`? Maybe like this?

if (settings)
{
  // notice_settings
  // set encoding
}
else
{
  // notice_settings with default values?
  // set encoding with default values
}


Arun Giridhar <arungiridhar>
Group Member
Tue 29 Nov 2022 07:36:22 AM UTC, comment #2: 

Would it be better to fall back to the default value instead of throwing an error in this case?

diff -r 7a0702b1d19c libgui/src/m-editor/file-editor-tab.cc
--- a/libgui/src/m-editor/file-editor-tab.cc        Mon Nov 28 19:56:21 2022 -0800
+++ b/libgui/src/m-editor/file-editor-tab.cc        Tue Nov 29 08:13:15 2022 +0100
@@ -272,7 +272,9 @@
       notice_settings (settings, true);

     // encoding, not updated with the settings
-    m_encoding = settings->value (ed_default_enc.key, "UTF-8").toString ();
+    m_encoding = settings
+                 ? settings->value (ed_default_enc.key, "UTF-8").toString ()
+                 : "UTF-8";
     m_enc_indicator->setText (m_encoding);
     // no changes in encoding yet
     m_new_encoding = m_encoding;


Markus Mützel <mmuetzel>
Group administrator
Tue 29 Nov 2022 04:26:36 AM UTC, comment #1: 

I don't think we want to use error_unless for this kind of problem in the GUI.  If NDEBUG is defined, it will be treated as a no-op, and any error it generates requires that the GUI is functional enough to display messages in the command window. 

Instead, I think we should be using a QMessageBox for these kinds of errors in the GUI.  And if this kind of check needs to happen frequently, then we should fix the resource manager's "get_settings" function to ensure that it will either return a valid pointer or handle the error itself?  Or maybe there is a better way to retrieve settings using signal/slot connections?

I'm wondering whether it is possible that this code (or something similar) could be the reason that people report that Octave no longer starts for them?  If Octave is trying to reload a file editor tab at startup and something goes wrong with accessing the settings in this constructor, Octave could crash with no useful error message displayed.

Also, as a minor point of style, we generally prefer to write "if (ptr)" instead of "if (ptr != nullptr)".

John W. Eaton <jwe>
Group administrator
Mon 28 Nov 2022 10:05:41 PM UTC, original submission:  

A static analyzer found a null pointer dereference with this code in file-editor-tab.cc:

   270            gui_settings *settings = rmgr.get_settings ();
   271            if (settings)
   272              notice_settings (settings, true);
   273
   274            // encoding, not updated with the settings
   275            m_encoding = settings->value (ed_default_enc.key, "UTF-8").toString ();


The problem is that `settings` is being checked for nullptr for only the `notice_settings` use but not for the `m_encoding = settings->value` use, so the second case could potentially dereference a nullptr.

The following change fixes it for me, to throw an error if `settings` is null, but this change needs review and feedback.


diff -r 785ac0d19116 libgui/src/m-editor/file-editor-tab.cc
--- a/libgui/src/m-editor/file-editor-tab.cc    Mon Nov 28 15:28:03 2022 -0500
+++ b/libgui/src/m-editor/file-editor-tab.cc    Mon Nov 28 16:50:28 2022 -0500
@@ -268,8 +268,10 @@ namespace octave

     resource_manager& rmgr = m_octave_qobj.get_resource_manager ();
     gui_settings *settings = rmgr.get_settings ();
-    if (settings)
-      notice_settings (settings, true);
+
+    error_unless (settings != nullptr);  // guard against nullptr assignment
+
+    notice_settings (settings, true);

     // encoding, not updated with the settings
     m_encoding = settings->value (ed_default_enc.key, "UTF-8").toString ();


Arun Giridhar <arungiridhar>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files

 

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 rik5 (Posted a comment)
  • -email is unavailable- added by ttl (Posted a comment)
  • -email is unavailable- added by nrjank (Posted a comment)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by arungiridhar (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 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-02-05 mmuetzel StatusIn Progress Fixed
        Open/ClosedOpen Closed
        Fixed ReleaseNone 8.1.0
    2022-12-03 jwe StatusPatch Submitted In Progress
    2022-11-29 mmuetzel Attached File- Added bug63437-file-editor-tab-settings-nullptr.patch, #54035

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code