How not to handle a bug report
I recently submitted a bug report to Qt software, the results were less than impressive. One thing I’d like to make clear though is that Qt is an amazing library that I would recommend to any c++ software developer, I truly mean that. It is well designed, well documented, and generally works as advertised. On top of all of this, it is portable! It really just gets better and better. This is why I found the handling of my bug report so… underwhelming.
It all started with a compiler warning. I like to compile my gcc projects with:
-ansi -pedantic -W -Wall
and I like my projects to compile with no warnings. My personal feelings on this subject are that if you cannot find a way to reconcile the warning, then odds are you don’t understand what it is trying to warn you about. Anyway, on to the issue at hand. I added a macro call of QT_REQUIRE_VERSION to the main function in one of my projects, the goal of course is to give a run-time error if the program is run with a version of Qt that is too old, simple enough. Unfortunately, this caused a warning in my project which I found entirely unacceptable.
main.cpp: In function 'int main(int, char**)':
main.cpp:8: warning: format not a string literal and no format arguments
I’m sure that many of you can relate to the horror I felt after seeing this in my usually clean build. First I checked, then double-checked that the usage of the macro was the cause. Yup, commenting out the simple one-liner use of the macro made it go away. Did I use it right? well the call basically looks like this:
QT_REQUIRE_VERSION(argc, argv, "4.0.2")
Not much room for error there, and the only string parameter is passing a string literal. So what’s going on here? I figured it’s a pretty minor issue so I’d be patient. After waiting a while for someone to notice this, I finally decided to submit a bug. Two or so weeks went by then I finally saw that my bug had been resolved as “invalid”. Fine, my gut reaction was to check if I had misused the macro, so I’ll check out what they had to say.
I believe the warning is due to there being no semicolon after the call to QT_REQUIRE_VERSION. Note that the warning message specifies line 8 – the line after the QT_REQUIRE_VERSION call. The QT_REQUIRE_VERSION macro, like most macros, does not end with a semi-colon, so you have to use one after calling it.
I was thrilled that it was such a simple fix. How could I have been so dumb as to miss a semi-colon? An amateur mistake! So I thanked the bug assignee, mentioned that the docs didn’t have a semicolon, and went on my way. Of course, this wouldn’t be much of a post if things were that simple…
As you may have guessed the proposed solution had nearly zero effect on the presence of the warning. I have to say “nearly” there because it did change the line number it occurred on to be the line of the macro, instead of the line after. But the warning still remains, mocking me.
Let’s revisit the response I got (I’m adding emphasis).
I believe the warning is due to there being no semicolon after the call to QT_REQUIRE_VERSION…
Ahh, I see, so he just guessed! I think it’s time for a basic rule of bug handling to be established.
1. Actually test the bug and then test the solution before any resolution is given!
This may sound simple, but clearly, it didn’t happen here. He did not test the issue or its solution, he simply looked at my example code and made a wild guess. It would have taken him all of 1 minute to copy and paste my code into a test.cpp file and compile it with the flags I gave to see that the warning would remain. I looked for a way to re-open the bug, but could not, so I submitted a second one. This time I made it clear that this was a follow-up to the other bug report and that I had added the semicolon, which did not help. If you are wondering how clear I was, here’s a quote from my bug report:
Note: I’ve added a semicolon as recommended by the previous bug’s resolution. Which did change the warning’s line number to 7 from 8. But the warning still stands.
Later that day I saw on my phone an email from the Qt bug tracker saying the status had changed. Less than 24 hours, that’s a great turnaround time, maybe I was in luck. Nope, not this time, the same assignee still didn’t do a good job :-(. He assigned it to someone in documentation with the following comment (remember my bug report was about a compile-time warning, the documentation was a side issue).
The problem was a missing semi-colon after the macro call, but that what the doc example leads you to do.
The example in the docs should be updated to have a semi-colon after the macro call, otherwise following the example produces code that fails to compile and the compiler error message is not very helpful.
Great! I’m glad we could get the documentation squared away, but how about fixing the actual bug I reported!
2. Read the actual contents of the bug report. Don’t just glaze over it and assume you know what I meant. Read it.
I don’t think it is too much to ask that the assignee of the bug actually read the bug report before drawing conclusions. This was a complete disregard for the reported issue. At this point I was annoyed, so I decided to look into the issue myself. Here’s the macro in question:
#define QT_REQUIRE_VERSION(argc, argv, str) { QString s = QString::fromLatin1(str);\
QString sq = QString::fromLatin1(qVersion()); \
if ((sq.section(QChar::fromLatin1('.'),0,0).toInt()<<16)+\
(sq.section(QChar::fromLatin1('.'),1,1).toInt()<<8)+\
sq.section(QChar::fromLatin1('.'),2,2).toInt()<(s.section(QChar::fromLatin1('.'),0,0).toInt()<<16)+\
(s.section(QChar::fromLatin1('.'),1,1).toInt()<<8)+\
s.section(QChar::fromLatin1('.'),2,2).toInt()) { \
if (!qApp){ \
new QApplication(argc,argv); \
} \
QString s = QApplication::tr("Executable '%1' requires Qt "\
"%2, found Qt %3.").arg(qAppName()).arg(QString::fromLatin1(\
str)).arg(QString::fromLatin1(qVersion())); QMessageBox::critical(0, QApplication::tr(\
"Incompatible Qt Library Error"), s, QMessageBox::Abort, 0); qFatal(s.toLatin1().data()); }}
The first thing to note is that the whole thing is wrapped in brackets, meaning that the semicolon isn’t strictly required and thus a complete non-issue. Sure there are the subtle issues involving things like this:
if (<condition>) MACRO(a); else foo(a);
But this macro is meant to be used unconditionally as the first statement in the program, nothing major. Personally, I’d recommend wrapping the macro in a do {/*...*/} while(0), but that’s a different discussion. Anyway, did you catch the source of the warning? Took me a bit but the issue is the very last statement:
qFatal(s.toLatin1().data());
Basically, they’ve constructed a QString, getting the equivalent c-string and passing that to qFatal, seems harmless enough. Let’s look at the definition for qFatal:
Q_CORE_EXPORT void qFatal(const char *, ...) /* print fatal message and exit */
#if defined(Q_CC_GNU) && !defined(__INSURE__)
__attribute__ ((format (printf, 1, 2)))
#endif
;
Aha! qFatal is a printf style function, it even has the printf attribute when compiled with gcc. This basically means we can expect that values passed will likely be given to something like vsprintf, followed by an abort call. This function is called qFatal after all. We all know that it is good security practice to always have the first parameter of a printf function be a string literal to prevent format string vulnerability (at least we all should know…), so the warning is well-founded. Out of curiosity, I tried something like this:
QT_REQUIRE_VERSION(argc, argv, "4.7.0%n%n%n%n");
Boom! Segmentation fault (instead of the expected abort). That means that those %n’s are trampling memory, which is simply not good. Fortunately, the odds of this macro being passed a value provided by a user is pretty much zero, so there is no practical vulnerability here, but it is still an issue, one which has a simple solution. If we simply change the last statement to look like this:
qFatal("%s", s.toLatin1().data());
Then all of these issues go away. No warning, no format string crashes when you do naughty things, it just works correctly. I’ve added all of these details to my bug report, so far no response. I still love Trolltech and thank them for their wonderful product which I will continue to recommend to people and be a happy user of. But someone over there really just wasn’t doing their job well. Once I got frustrated enough to look, it took me all of 15 minutes to figure out exactly what was causing the warning and come up with a functional solution. Instead, my report has been assigned to documentation where it will likely sit for days until someone realizes it doesn’t belong there. Wish me luck, I’ve got my fingers crossed.