Bug 366135 - audex-0.79: read uninit data ?
Summary: audex-0.79: read uninit data ?
Status: RESOLVED FIXED
Alias: None
Product: kde
Classification: I don't know
Component: general (show other bugs)
Version: unspecified
Platform: Compiled Sources Linux
: NOR normal
Target Milestone: ---
Assignee: Unassigned bugs mailing-list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-07-26 15:29 UTC by dcb314
Modified: 2016-07-28 08:53 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description dcb314 2016-07-26 15:29:04 UTC
utils/patternparser.cpp:221]: (error) Uninitialized variable: ok

Source code is

        bool ok;
        if (!atts.value("x").isEmpty()) x = atts.value("x").toInt(&ok);
        if (!ok) x = -1;

Also,

utils/patternparser.cpp:234]: (style) Redundant condition: cover. '!cover || (cover && cover.isEmpty())' is equivalent to '!cover || cover.isEmpty()'


Reproducible: Always
Comment 1 Christoph Feck 2016-07-27 11:43:08 UTC
QString::toInt() only writes to the variable pointed to, so the code for the first issue is fine.
Comment 2 dcb314 2016-07-27 12:24:04 UTC
>code for the first issue is fine.

Perhaps I've been less than clear. Type bool doesn't have a default constructor.

The if statement means that local variable "ok" is only optionally written to.

Hence the read of un-init memory.
Comment 3 Leslie Zhai 2016-07-28 01:54:21 UTC
Hi dcb314,

> QString::toInt() only writes to the variable pointed to, so the code for the first issue is fine.

Please pay more attention to int QString::toInt(bool *ok, int base) const
If a conversion error occurs, *ok is set to false; otherwise *ok is set to true. http://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/tools/qstring.cpp#n6418

Much deeper please see http://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/tools/qlocale.cpp#n3277

I know references are syntactic sugar, so code is easier to read and write, and you can Google with C++ reference vs pointer ;-)

cover was set by setCover https://github.com/KDE/audex/commit/bd2d7578aa9ee9cd38cb4a55df256200dbe71690
Comment 4 dcb314 2016-07-28 06:10:33 UTC
So what happens in the case where the if fails, so the call to toInt doesn't get executed ?

The code then does this:

 bool ok;
 if (0) whatever;
 if (!ok) x = 1;

or are you saying that this project has a special version of C++ standard type bool ?
Comment 5 Leslie Zhai 2016-07-28 07:35:18 UTC
I just checked the code here https://github.com/KDE/audex/commit/bd2d7578aa9ee9cd38cb4a55df256200dbe71690#diff-1bf58c9ce26d902a32dfbd7d88b39228R67

> So what happens in the case where the if fails, so the call to toInt doesn't get executed ?

You are right! here *bool ok* leads C++ uninitialized bool issue! 

Can you check through the audex kf5 branch, and upload some patch to git REVIEW https://git.reviewboard.kde.org/dashboard/ and assign the people to lesliezhai, I will review it, thanks a lot!
Comment 6 Leslie Zhai 2016-07-28 07:46:42 UTC
Git commit da9f3c23b6609a014538d3c21f8bab6f770d4092 by Leslie Zhai.
Committed on 28/07/2016 at 07:44.
Pushed by lesliezhai into branch 'kf5'.

Fix uninitialized issue

Because int QString::toInt(bool* ok, int base) const often return ZERO
when failed to convert string to int - *ok is false.

M  +10   -5    utils/patternparser.cpp

http://commits.kde.org/audex/da9f3c23b6609a014538d3c21f8bab6f770d4092
Comment 7 dcb314 2016-07-28 08:02:22 UTC
Weird. Code looks fixed, then re-broken by latest commit. 
Strange that Leslie had the right idea at 7:35, but then seems to
have re-broke it a few minutes later.

Suggested fix is

 bool ok = false;

*ONLY*. No change in any later code required.
Comment 8 Leslie Zhai 2016-07-28 08:08:26 UTC
> Weird. Code looks fixed, then re-broken by latest commit

nope, it followed the orignial author's mind:

1. initialied x and y to -1
2. if XML stringbuffer x !isEmpty, then convert the string to int
3. if failed to convert, set x = -1, because QString::toInt() return ZERO when failure
Comment 9 Leslie Zhai 2016-07-28 08:14:31 UTC
And MyString (acts like QString) testcase https://github.com/xiangzhai/leetcode/blob/master/src/string/teststring.cpp
Comment 10 dcb314 2016-07-28 08:41:46 UTC
I've had a few goes at trying to explain a trivial problem, without much success.

I think I'd better stop now. Have fun !
Comment 11 Leslie Zhai 2016-07-28 08:53:43 UTC
Uninitialized issue http://stackoverflow.com/questions/4879045/fun-with-uninitialized-variables-and-compiler-gcc is depends:

1. bool ok; <- without initlized
    if (ok) {} then lead uninitialized issue!

2. bool ok;
QString::toInt(&ok);
// *ok Pointer has been pointed to some address
if (ok) {} NOT issue!