QT deployed executable crashes on some computers

[JonB]

@Lati said in QT deployed executable crashes on some computers:

It is very easy to test by writing a small application.

char *p = getenv("ProgramXROOT");
// or
char *p = getenv("AnythingElseWhichDoesntExist");

won't crash. It will set p to NULL/nullptr.

If the program continues and does not check for that, assuming that p will not be null, it may crash. That is all @Pablo-J-Rogina and I are saying.

[fcarney]

@Lati said in QT deployed executable crashes on some computers:

programXRootDirectory

It would be interesting to know what the data type is for this variable. I could not get QString to crash or misbehave.

{
        char* null = nullptr;

        QString test = "hallo";
        qInfo() << "before init null";
        test = QString(null);
        qInfo() << test;
        qInfo() << "after init null";

        QString test2 = "hallo again";
        qInfo() << "before assign null";
        test2 = null;
        qInfo() << test2;
        qInfo() << "after assign null";
    }

[Lati]

@fcarney
You can reproduce the issue with the following example (at least it crashes on my computer:)):

main.cpp

#include "mainwindow.h"
#include <QApplication>

int main(int argc, char *argv[])
{
    QApplication a(argc, argv);
    MainWindow w;
    w.show();
    return a.exec();
}

mainWindow.h:

#ifndef MAINWINDOW_H
#define MAINWINDOW_H

#include <QMainWindow>

QT_BEGIN_NAMESPACE
namespace Ui { class MainWindow; }
QT_END_NAMESPACE

class MainWindow : public QMainWindow
{
    Q_OBJECT

public:
    MainWindow(QWidget *parent = nullptr);
    ~MainWindow();

    // Senex root directory
    std::string notExistEnvVar;

private:
    Ui::MainWindow *ui;
};
#endif // MAINWINDOW_H

mainWindow.cpp

#include "mainwindow.h"
#include "ui_mainwindow.h"

MainWindow::MainWindow(QWidget *parent)
    : QMainWindow(parent)
    , ui(new Ui::MainWindow)
{
    ui->setupUi(this);

    notExistEnvVar = getenv("notExistEnvVar");
}

MainWindow::~MainWindow()
{
    delete ui;
}

Compiled with MSVC2015 64bit.

[fcarney]

You are getting an exception because you are assigning nullptr to std::string. Assign to

char* ptr = getenv("notExistEnvVar");
if(ptr != nullptr)
  notExistEnvVar = ptr;

getenv is not crashing

Edit:
or use QString

QString str = getenv("notExistEnvVar");

[JonB]

@fcarney Thank you :)

[JonB]

For std::string s = nullptr, see https://stackoverflow.com/questions/10771864/assign-a-nullptr-to-a-stdstring-is-safe/10771938. Accepted answer:

Requires: s shall not be a null pointer.

Since the standard does not ask the library to throw an exception when this particular requirement is not met, it would appear that passing a null pointer provoked undefined behavior.

[fcarney]

@JonB said in QT deployed executable crashes on some computers:

undefined

!!!UNDEFINED!!!
The end is nigh!

Good find!

[Pablo J. Rogina]

@JonB said in QT deployed executable crashes on some computers:

won't crash. It will set p to NULL/nullptr.

The problem is not the assignment... the problem comes once you have assigned NULL to the pointer and then you try using it

[Lati]

@fcarney

Obviously, the person who wrote the library didn't know this (to be honest, I didn't know it as well, I have never used getenv). And this is how bugs happened, right? :)

But interesting thing is that there is no sign about the crash and no-one had an idea about it (including me). I would still look at the error somewhere else if I wouldn't test the code on one of the problem computer.

Anyway, thank you for the clarification.

[JonB]

@Lati
For the record, getenv() has been in the C runtime libraries since the 1970s(!) There has to be a way for it to tell you that the selected environment variable does not exist (without crashing!), and that is of course by returning 0. I and millions of other coders have been using that behaviour ever since :)

In your case, whoever wrote the code which does not check for that will doubtless have been working in an environment where that ProgramXROOT variable did always exist, and hence never witnessed the unanticipated behaviour.

OOI, how have you resolved this? Did you actually change that library's code to fix and recompile, or have you just told your end users they must have that environment variable defined?

[jsulm]

@JonB @Lati
Yes, "man getenv" shows this:
"RETURN VALUE
The getenv() function returns a pointer to the value in the environment, or NULL if there is no match."
It is the responsibility of the caller to check its return value and reacting accordingly...

[Lati]

@JonB I am not one of that millions of coders (or even if I used it, I never had problems).

In the code, I replaced the getenv part using _dupenv_s as following (which will be valid with the next release):

    char* buf = nullptr;
    size_t sz = 0;
    if (_dupenv_s(&buf, &sz, "programXROOT") == 0 && buf != nullptr)
    {
        programXRootDirectory = buf;
        free(buf);
    }
    else
    {
        QMessageBox msgBox;
        msgBox.setText("Environmental variable is missing!");
        msgBox.exec();
    }

Actually, after installation of this application, user has to create this environmental variable before starting the application. And seems like all the users have created the variable and there was no issue so far. But if a user would forget about this, it would be nightmare to find out the reason.

[JonB]

@Lati
Again for the record, the _dupenv_s (which btw is MSVC-only) you mention has nothing to do with the issue you are talking about. The extra parameters it takes are to do with copying the value, if found, into your own buffer. If it's not found:

If the variable is not found, then buffer is set to NULL, numberOfElements is set to 0, and the return value is 0 because this situation is not considered to be an error condition.

So the issue will rear its head again if code later tries to dereference the buf from the &buf passed in. Just as with the return result from getenv().

Purely as a by-the-by, I presume you are aware in the code you show (which perhaps is only intended as an example):

        programXRootDirectory = buf;
        free(buf);

This would not be a good idea --- programXRootDirectory is left pointing to freed memory! You'll get a different error/crash/behaviour if you then dereference that :)