Re: auto QString(Builder) considered VERY HARMFUL
On Thursday, 27 September 2018 12:30:30 PDT Elvis Angelaccio wrote: > is it possible to fix Qt instead? No, the C++ language needs to be fixed. Search for "operator auto" for proposals. -- Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org Software Architect - Intel Open Source Technology Center
Re: auto QString(Builder) considered VERY HARMFUL -> use clazy, especially before releases
Am Freitag, 28. September 2018, 11:15:52 CEST schrieb Friedrich W. H. Kossebau: > Am Freitag, 28. September 2018, 01:03:01 CEST schrieb Albert Astals Cid: > > El dijous, 27 de setembre de 2018, a les 21:01:13 CEST, Friedrich W. > > H. > Kossebau va escriure: > > > One would recommend to run clazy over your code at least before > > > releases, to catch all kind of potential issues :) > > > > Or since this is a crasher, just run your app and it'll crash? > > > > Or even better, add autotests that exercise the code and they'll > > crash too? > s/Or/And/ ? :) > > BTW, not necessarily a crasher, the references can point to random > data which still can get interpreted into proper QString data, which > will "only" deliver bogus string results (or by chance even "correct" > one if it's still the old data at the used memory), but not trigger a > crash, as no data is changed, as the reference is only read from. At > least from what I remember to have seen. > > ASan seems to help here though, I think I fixed at least one such bug > due to ASan throwing up use-after-free or similar on KDE CI and thus > pointing out the issue. > To add to that, the most recent instance I fixed was from 2016 and was triggered by some new changes introducing (AFAICS) no fault of their own. They just shuffled things around enough to trigger the latent bug. > Cheers > Friedrich
Re: auto QString(Builder) considered VERY HARMFUL -> use clazy, especially before releases
Am Freitag, 28. September 2018, 01:03:01 CEST schrieb Albert Astals Cid: > El dijous, 27 de setembre de 2018, a les 21:01:13 CEST, Friedrich W. H. Kossebau va escriure: > > One would recommend to run clazy over your code at least before releases, > > to catch all kind of potential issues :) > > Or since this is a crasher, just run your app and it'll crash? > > Or even better, add autotests that exercise the code and they'll crash too? s/Or/And/ ? :) BTW, not necessarily a crasher, the references can point to random data which still can get interpreted into proper QString data, which will "only" deliver bogus string results (or by chance even "correct" one if it's still the old data at the used memory), but not trigger a crash, as no data is changed, as the reference is only read from. At least from what I remember to have seen. ASan seems to help here though, I think I fixed at least one such bug due to ASan throwing up use-after-free or similar on KDE CI and thus pointing out the issue. Cheers Friedrich
Re: auto QString(Builder) considered VERY HARMFUL -> use clazy, especially before releases
El dijous, 27 de setembre de 2018, a les 21:01:13 CEST, Friedrich W. H. Kossebau va escriure: > Am Donnerstag, 27. September 2018, 20:08:54 CEST schrieb Andreas Hartmetz: > > Today I fixed the third or so crash in KDE software due to the following > > pattern: > > > > const auto str = someString + anotherString; > > > > What happens is that the type of str ends up being QStringBuilder > > instead of QString. The QStringBuilder is destroyed after the semicolon, > > and all access to "str" produces undefined behavior. > > The QStringBuilder actually survives as long as str is in scope, but the > references it potentially takes from someString or anotherString (if e.g. > some > temporary QString object as returned from QStringLiteral or other QString- > returning methods) will be no longer valid after the scope of the expression > is left. > So if str is finally resolved to a real QString, those references are > dangling > and non-funny things happen. > > > While I'm not a particularly big fan of using auto to hide variable > > types anyway, this kind of usage is just wrong and must be avoided. > > Please take care. > > Care can be done e.g. by deploying clazy with auto-unexpected-qstringbuilder: > > clazy-standalone \ > -checks=auto-unexpected-qstringbuilder -p > > See > https://phabricator.kde.org/source/clazy/browse/master/docs/checks/README-auto-unexpected-qstringbuilder.md?as=remarkup > https://phabricator.kde.org/source/clazy/browse/master/README.md?as=remarkup > > One would recommend to run clazy over your code at least before releases, to > catch all kind of potential issues :) Or since this is a crasher, just run your app and it'll crash? Or even better, add autotests that exercise the code and they'll crash too? Cheers, Albert > > Cheers > Friedrich > > >
Re: auto QString(Builder) considered VERY HARMFUL -> use clazy /w kdevelop
Darn, forgot some obvious promo bits... see below: Am Donnerstag, 27. September 2018, 21:01:13 CEST schrieb Friedrich W. H. Kossebau: > Am Donnerstag, 27. September 2018, 20:08:54 CEST schrieb Andreas Hartmetz: > > Please take care. > > Care can be done e.g. by deploying clazy with > auto-unexpected-qstringbuilder: > > clazy-standalone \ > -checks=auto-unexpected-qstringbuilder -p > > See > https://phabricator.kde.org/source/clazy/browse/master/docs/checks/README-au > to-unexpected-qstringbuilder.md?as=remarkup > https://phabricator.kde.org/source/clazy/browse/master/README.md?as=remarku > p > > One would recommend to run clazy over your code at least before releases, to > catch all kind of potential issues :) If you are using KDevelop, upcoming version 5.3 features clazy integration. So it is just a matter of selecting "auto-unexpected-qstringbuilder" in the config UI, and then triggering a run of Clazy over the whole project, some subdir or just a single source file :) Test yourself by getting a Nightly Build of what very soon is becoming KDevelop 5.3 from here: https://www.kdevelop.org/download Needs a separate clazy installation. Well stuffed distribution repos should provide packages for you (like for Arch Linux, openSUSE Tumbleweed or OpenMandriva). Building clazy yourself works as well, just needs careful reading of instructions about clazy-standalone binary location in the README.md file. And building with just -j 1, compiling against clang/llvm makes the compiler eat lots of your memory :) Cheers Friedrich
Re: auto QString(Builder) considered VERY HARMFUL
On giovedì 27 settembre 2018 20:08:54 CEST, Andreas Hartmetz wrote: Hello, Today I fixed the third or so crash in KDE software due to the following pattern: const auto str = someString + anotherString; What happens is that the type of str ends up being QStringBuilder instead of QString. The QStringBuilder is destroyed after the semicolon, and all access to "str" produces undefined behavior. While I'm not a particularly big fan of using auto to hide variable types anyway, this kind of usage is just wrong and must be avoided. Please take care. Hi, is it possible to fix Qt instead? Cheers, Andreas Cheers, Elvis
Re: auto QString(Builder) considered VERY HARMFUL -> use clazy, especially before releases
Am Donnerstag, 27. September 2018, 20:08:54 CEST schrieb Andreas Hartmetz: > Today I fixed the third or so crash in KDE software due to the following > pattern: > > const auto str = someString + anotherString; > > What happens is that the type of str ends up being QStringBuilder > instead of QString. The QStringBuilder is destroyed after the semicolon, > and all access to "str" produces undefined behavior. The QStringBuilder actually survives as long as str is in scope, but the references it potentially takes from someString or anotherString (if e.g. some temporary QString object as returned from QStringLiteral or other QString- returning methods) will be no longer valid after the scope of the expression is left. So if str is finally resolved to a real QString, those references are dangling and non-funny things happen. > While I'm not a particularly big fan of using auto to hide variable > types anyway, this kind of usage is just wrong and must be avoided. > Please take care. Care can be done e.g. by deploying clazy with auto-unexpected-qstringbuilder: clazy-standalone \ -checks=auto-unexpected-qstringbuilder -p See https://phabricator.kde.org/source/clazy/browse/master/docs/checks/README-auto-unexpected-qstringbuilder.md?as=remarkup https://phabricator.kde.org/source/clazy/browse/master/README.md?as=remarkup One would recommend to run clazy over your code at least before releases, to catch all kind of potential issues :) Cheers Friedrich
Re: auto QString(Builder) considered VERY HARMFUL
I would be awesome if clazy detected this :) Em qui, 27 de set de 2018 às 15:09, Andreas Hartmetz escreveu: > > Hello, > > Today I fixed the third or so crash in KDE software due to the following > pattern: > > const auto str = someString + anotherString; > > What happens is that the type of str ends up being QStringBuilder > instead of QString. The QStringBuilder is destroyed after the semicolon, > and all access to "str" produces undefined behavior. > While I'm not a particularly big fan of using auto to hide variable > types anyway, this kind of usage is just wrong and must be avoided. > Please take care. > > Cheers, > Andreas > > -- Daniel Nicoletti KDE Developer - http://dantti.wordpress.com
auto QString(Builder) considered VERY HARMFUL
Hello, Today I fixed the third or so crash in KDE software due to the following pattern: const auto str = someString + anotherString; What happens is that the type of str ends up being QStringBuilder instead of QString. The QStringBuilder is destroyed after the semicolon, and all access to "str" produces undefined behavior. While I'm not a particularly big fan of using auto to hide variable types anyway, this kind of usage is just wrong and must be avoided. Please take care. Cheers, Andreas
