bug report + fix for out of bounds memory access

Next Topic
 
classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

bug report + fix for out of bounds memory access

Nam Nguyen
Hi,

On OpenBSD qt5 eliot segfaults when opening any menu due to out of
bounds memory access. Here is a diff to fix it.

- I discovered a new segfault when opening any menu, resolved by
  patch-qt_main_cpp. eliot adds its own MyApplication class with
  superclass QApplication. MyApplication's constructor wrongly passes in
  the argc argument by value because these superclasses store argc as
  reference to this argument. However, this argument expires after the
  constructor returns.

  Opening a menu uses a codepath that eventually calls arguments().
  x11/qt/qtbase's ${WRKSRC}/src/corelib/kernel/qcoreapplication.cpp
  QCoreApplication::arguments() returns command-line arguments as a list
  of strings.
 
  const int ac = self->d_func()->argc;
  char ** const av = self->d_func()->argv;

  gdb showed that ac had a distinct, large value and different address
  from main()'s argc (argc = 1 and argv[0] = /usr/local/bin/eliot
  typically). The large ac caused reading past the end of av.

  To resolve, heed the warning in the documentation, "Warning: The data
  referred to by argc and argv must stay valid for the entire lifetime
  of the QApplication object." Pass argc from main() by reference to
  MyApplication and all of its superclasses, which already store
  references to argc.

- diff inline:

Resolve out of bounds memory access when opening any menu

Warning: The data referred to by argc and argv must stay valid for the entire
lifetime of the QApplication object.[1]

Pass argc by reference instead of by value because main()'s argc will stay
valid. Before, argc copied by value to this constructor expired after the
constructor returned. arguments()[2] eventually used this expired argc to read
past the end of argv.

See also:
[1] https://doc.qt.io/qt-5/qapplication.html#QApplication
[2] https://doc.qt.io/qt-5/qcoreapplication.html#arguments

diff refs/heads/master refs/heads/fix-reference
blob - 568817278e5e03726eff2698a812f90246b8d5b4
blob + da05442abdebbffa5f4d67860c6327de9ec1d13e
--- qt/main.cpp
+++ qt/main.cpp
@@ -54,7 +54,7 @@ static void bt_sighandler(int);
 class MyApplication : public QApplication
 {
 public:
-    MyApplication(int argc, char **argv)
+    MyApplication(int &argc, char **argv)
         : QApplication(argc, argv)
     {}
 

_______________________________________________
Eliot-dev mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/eliot-dev
Reply | Threaded
Open this post in threaded view
|

Re: bug report + fix for out of bounds memory access

Olivier Teuliere
Hi Nam,

I have just applied your patch.
Thanks a lot for the explanation and for the fix!

Cheers,
Olivier

On Tue, Jun 23, 2020 at 12:12 PM Nam Nguyen <[hidden email]> wrote:
Hi,

On OpenBSD qt5 eliot segfaults when opening any menu due to out of
bounds memory access. Here is a diff to fix it.

- I discovered a new segfault when opening any menu, resolved by
  patch-qt_main_cpp. eliot adds its own MyApplication class with
  superclass QApplication. MyApplication's constructor wrongly passes in
  the argc argument by value because these superclasses store argc as
  reference to this argument. However, this argument expires after the
  constructor returns.

  Opening a menu uses a codepath that eventually calls arguments().
  x11/qt/qtbase's ${WRKSRC}/src/corelib/kernel/qcoreapplication.cpp
  QCoreApplication::arguments() returns command-line arguments as a list
  of strings.

  const int ac = self->d_func()->argc;
  char ** const av = self->d_func()->argv;

  gdb showed that ac had a distinct, large value and different address
  from main()'s argc (argc = 1 and argv[0] = /usr/local/bin/eliot
  typically). The large ac caused reading past the end of av.

  To resolve, heed the warning in the documentation, "Warning: The data
  referred to by argc and argv must stay valid for the entire lifetime
  of the QApplication object." Pass argc from main() by reference to
  MyApplication and all of its superclasses, which already store
  references to argc.

- diff inline:

Resolve out of bounds memory access when opening any menu

Warning: The data referred to by argc and argv must stay valid for the entire
lifetime of the QApplication object.[1]

Pass argc by reference instead of by value because main()'s argc will stay
valid. Before, argc copied by value to this constructor expired after the
constructor returned. arguments()[2] eventually used this expired argc to read
past the end of argv.

See also:
[1] https://doc.qt.io/qt-5/qapplication.html#QApplication
[2] https://doc.qt.io/qt-5/qcoreapplication.html#arguments

diff refs/heads/master refs/heads/fix-reference
blob - 568817278e5e03726eff2698a812f90246b8d5b4
blob + da05442abdebbffa5f4d67860c6327de9ec1d13e
--- qt/main.cpp
+++ qt/main.cpp
@@ -54,7 +54,7 @@ static void bt_sighandler(int);
 class MyApplication : public QApplication
 {
 public:
-    MyApplication(int argc, char **argv)
+    MyApplication(int &argc, char **argv)
         : QApplication(argc, argv)
     {}


_______________________________________________
Eliot-dev mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/eliot-dev


_______________________________________________
Eliot-dev mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/eliot-dev