QtMir

Merge lp:~aacid/qtmir/fix-heap-use-after-free into lp:qtmir

  1. fix-heap-use-after-free
  2. Merge into trunk
Proposed by Albert Astals Cid
Status: Rejected
Rejected by: Michał Sawicz
Proposed branch: lp:~aacid/qtmir/fix-heap-use-after-free
Merge into: lp:qtmir
Diff against target: 13 lines (+2/-1)
1 file modified
src/modules/Unity/Application/desktopfilereader.cpp (+2/-1)
To merge this branch: bzr merge lp:~aacid/qtmir/fix-heap-use-after-free
Reviewer Review Type Date Requested Status
Michał Sawicz Disapprove
Unity8 CI Bot (community) continuous-integration Needs Fixing
Lukáš Tinkl (community) Approve
Review via email: mp+292644@code.launchpad.net

Commit message

Fix heap-use-after-free reported by ASAN

You can not keep the constData() of a temporary bytearray

ASAN goes with

==30749==ERROR: AddressSanitizer: heap-use-after-free on address 0x6060000039f8 at pc 0x7f1367f582fd bp 0x7ffcc9238c40 sp 0x7ffcc92383e8
READ of size 26 at 0x6060000039f8 thread T0
    #0 0x7f1367f582fc (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x472fc)
    #1 0x7f13663bf6d8 in g_str_equal (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x396d8)
    #2 0x7f13663bebae in g_hash_table_lookup (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x38bae)
    #3 0x7f13663cb04d in g_key_file_has_key (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4504d)
    #4 0x7f1367c69be8 in qtmir::DesktopFileReader::splashTitle() const /home/tsdgeos_work/phablet/qtmir/set-display-config/src/modules/Unity/Application/desktopfilereader.cpp:169
    #5 0x421208 in DesktopFileReader_testReadsLocalizedDesktopFile_Test::TestBody() /home/tsdgeos_work/phablet/qtmir/set-display-config/tests/modules/DesktopFileReader/desktopfilereader_test.cpp:86
    #6 0x47021e in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /usr/src/gtest/src/gtest.cc:2078
    #7 0x47021e in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /usr/src/gtest/src/gtest.cc:2114
    #8 0x45e9cd in testing::Test::Run() /usr/src/gtest/src/gtest.cc:2151
    #9 0x45eca7 in testing::TestInfo::Run() /usr/src/gtest/src/gtest.cc:2326
    #10 0x45ee9c in testing::TestCase::Run() /usr/src/gtest/src/gtest.cc:2444
    #11 0x45f553 in testing::internal::UnitTestImpl::RunAllTests() /usr/src/gtest/src/gtest.cc:4315
    #12 0x470e1e in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /usr/src/gtest/src/gtest.cc:2078
    #13 0x470e1e in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /usr/src/gtest/src/gtest.cc:2114
    #14 0x45fe14 in testing::UnitTest::Run() /usr/src/gtest/src/gtest.cc:3926
    #15 0x41808a in RUN_ALL_TESTS() /usr/include/gtest/gtest.h:2288
    #16 0x41808a in main /usr/src/gtest/src/gtest_main.cc:37
    #17 0x7f1366bc482f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #18 0x418128 in _start (/home/tsdgeos_work/phablet/qtmir/set-display-config/build/tests/modules/DesktopFileReader/desktop_file_reader_test+0x418128)

0x6060000039f8 is located 24 bytes inside of 64-byte region [0x6060000039e0,0x606000003a20)
freed by thread T0 here:
    #0 0x7f1367fa924a in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x9824a)
    #1 0x7f1367c69b96 in QTypedArrayData<char>::deallocate(QArrayData*) /usr/include/x86_64-linux-gnu/qt5/QtCore/qarraydata.h:222
    #2 0x7f1367c69b96 in QByteArray::~QByteArray() /usr/include/x86_64-linux-gnu/qt5/QtCore/qbytearray.h:459
    #3 0x7f1367c69b96 in qtmir::DesktopFileReader::splashTitle() const /home/tsdgeos_work/phablet/qtmir/set-display-config/src/modules/Unity/Application/desktopfilereader.cpp:168
    #4 0x421208 in DesktopFileReader_testReadsLocalizedDesktopFile_Test::TestBody() /home/tsdgeos_work/phablet/qtmir/set-display-config/tests/modules/DesktopFileReader/desktopfilereader_test.cpp:86
    #5 0x47021e in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /usr/src/gtest/src/gtest.cc:2078
    #6 0x47021e in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /usr/src/gtest/src/gtest.cc:2114
    #7 0x45e9cd in testing::Test::Run() /usr/src/gtest/src/gtest.cc:2151
    #8 0x45eca7 in testing::TestInfo::Run() /usr/src/gtest/src/gtest.cc:2326
    #9 0x45ee9c in testing::TestCase::Run() /usr/src/gtest/src/gtest.cc:2444
    #10 0x45f553 in testing::internal::UnitTestImpl::RunAllTests() /usr/src/gtest/src/gtest.cc:4315
    #11 0x470e1e in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /usr/src/gtest/src/gtest.cc:2078
    #12 0x470e1e in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /usr/src/gtest/src/gtest.cc:2114
    #13 0x45fe14 in testing::UnitTest::Run() /usr/src/gtest/src/gtest.cc:3926
    #14 0x41808a in RUN_ALL_TESTS() /usr/include/gtest/gtest.h:2288
    #15 0x41808a in main /usr/src/gtest/src/gtest_main.cc:37
    #16 0x7f1366bc482f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)

previously allocated by thread T0 here:
    #0 0x7f1367fa98ca in realloc (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x988ca)
    #1 0x7f13677cb794 in QByteArray::reallocData(unsigned int, QFlags<QArrayData::AllocationOption>) (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0xa9794)
    #2 0xc533d48a9edeb0ff (<unknown module>)

Description of the change

Fix heap-use-after-free when reading localized X-Ubuntu-Splash-Title from the desktop file

To post a comment you must log in.
Revision history for this message
Lukáš Tinkl (lukas-kde) wrote :

LGTM, looks valid to me

review: Approve
Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :

FAILED: Continuous integration, rev:471
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/183/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/1399
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1368
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/1368
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/1368
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/1368
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/1368/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial/1368
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial/1368/artifact/output/*zip*/output.zip
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1368/console
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=vivid+overlay/1368
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=vivid+overlay/1368/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial/1368
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial/1368/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1368
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1368/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=vivid+overlay/1368
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=vivid+overlay/1368/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial/1368
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial/1368/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/1368
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/1368/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/183/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Michał Sawicz (saviq) wrote :

+N src/modules/Unity/Application/desktopfilereader.cpp.OTHER
Contents conflict in src/modules/Unity/Application/desktopfilereader.cpp
1 conflicts encountered.

review: Needs Fixing
Revision history for this message
Michał Sawicz (saviq) wrote :

Right, we've done away with DesktopFileReader :)

review: Disapprove

Unmerged revisions

471. By Albert Astals Cid

Fix heap-use-after-free reported by ASAN

You can not keep the constData() of a temporary bytearray

ASAN goes with

==30749==ERROR: AddressSanitizer: heap-use-after-free on address 0x6060000039f8 at pc 0x7f1367f582fd bp 0x7ffcc9238c40 sp 0x7ffcc92383e8
READ of size 26 at 0x6060000039f8 thread T0
    #0 0x7f1367f582fc (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x472fc)
    #1 0x7f13663bf6d8 in g_str_equal (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x396d8)
    #2 0x7f13663bebae in g_hash_table_lookup (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x38bae)
    #3 0x7f13663cb04d in g_key_file_has_key (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4504d)
    #4 0x7f1367c69be8 in qtmir::DesktopFileReader::splashTitle() const /home/tsdgeos_work/phablet/qtmir/set-display-config/src/modules/Unity/Application/desktopfilereader.cpp:169
    #5 0x421208 in DesktopFileReader_testReadsLocalizedDesktopFile_Test::TestBody() /home/tsdgeos_work/phablet/qtmir/set-display-config/tests/modules/DesktopFileReader/desktopfilereader_test.cpp:86
    #6 0x47021e in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /usr/src/gtest/src/gtest.cc:2078
    #7 0x47021e in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /usr/src/gtest/src/gtest.cc:2114
    #8 0x45e9cd in testing::Test::Run() /usr/src/gtest/src/gtest.cc:2151
    #9 0x45eca7 in testing::TestInfo::Run() /usr/src/gtest/src/gtest.cc:2326
    #10 0x45ee9c in testing::TestCase::Run() /usr/src/gtest/src/gtest.cc:2444
    #11 0x45f553 in testing::internal::UnitTestImpl::RunAllTests() /usr/src/gtest/src/gtest.cc:4315
    #12 0x470e1e in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /usr/src/gtest/src/gtest.cc:2078
    #13 0x470e1e in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /usr/src/gtest/src/gtest.cc:2114
    #14 0x45fe14 in testing::UnitTest::Run() /usr/src/gtest/src/gtest.cc:3926
    #15 0x41808a in RUN_ALL_TESTS() /usr/include/gtest/gtest.h:2288
    #16 0x41808a in main /usr/src/gtest/src/gtest_main.cc:37
    #17 0x7f1366bc482f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #18 0x418128 in _start (/home/tsdgeos_work/phablet/qtmir/set-display-config/build/tests/modules/DesktopFileReader/desktop_file_reader_test+0x418128)

0x6060000039f8 is located 24 bytes inside of 64-byte region [0x6060000039e0,0x606000003a20)
freed by thread T0 here:
    #0 0x7f1367fa924a in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x9824a)
    #1 0x7f1367c69b96 in QTypedArrayData<char>::deallocate(QArrayData*) /usr/include/x86_64-linux-gnu/qt5/QtCore/qarraydata.h:222
    #2 0x7f1367c69b96 in QByteArray::~QByteArray() /usr/include/x86_64-linux-gnu/qt5/QtCore/qbytearray.h:459
    #3 0x7f1367c69b96 in qtmir::DesktopFileReader::splashTitle() const /home/tsdgeos_work/phablet/qtmir/set-display-config/src/modules/Unity/Application/desktopfilereader.cpp:168
    #4 0x421208 in DesktopFileReader_testReadsLocalizedDesktopFile_Test::TestBody() /home/tsdgeos_work/phablet/qtmir/set-display-config/tests/modules/DesktopFileReader/desktopfilereader_test.cpp:86
    #5 0x47021e in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /usr/src/gtest/src/gtest.cc:2078
    #6 0x47021e in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /usr/src/gtest/src/gtest.cc:2114
    #7 0x45e9cd in testing::Test::Run() /usr/src/gtest/src/gtest.cc:2151
    #8 0x45eca7 in testing::TestInfo::Run() /usr/src/gtest/src/gtest.cc:2326
    #9 0x45ee9c in testing::TestCase::Run() /usr/src/gtest/src/gtest.cc:2444
    #10 0x45f553 in testing::internal::UnitTestImpl::RunAllTests() /usr/src/gtest/src/gtest.cc:4315
    #11 0x470e1e in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /usr/src/gtest/src/gtest.cc:2078
    #12 0x470e1e in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /usr/src/gtest/src/gtest.cc:2114
    #13 0x45fe14 in testing::UnitTest::Run() /usr/src/gtest/src/gtest.cc:3926
    #14 0x41808a in RUN_ALL_TESTS() /usr/include/gtest/gtest.h:2288
    #15 0x41808a in main /usr/src/gtest/src/gtest_main.cc:37
    #16 0x7f1366bc482f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)

previously allocated by thread T0 here:
    #0 0x7f1367fa98ca in realloc (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x988ca)
    #1 0x7f13677cb794 in QByteArray::reallocData(unsigned int, QFlags<QArrayData::AllocationOption>) (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0xa9794)
    #2 0xc533d48a9edeb0ff (<unknown module>)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/modules/Unity/Application/desktopfilereader.cpp'
2--- src/modules/Unity/Application/desktopfilereader.cpp 2015-11-20 12:06:54 +0000
3+++ src/modules/Unity/Application/desktopfilereader.cpp 2016-04-22 14:42:25 +0000
4@@ -165,7 +165,8 @@
5 for (QString locale: locales) {
6 // Desktop files use local specifiers with underscore separators but Qt uses hyphens
7 locale = locale.replace('-', '_');
8- const char* key = keyTemplate.arg(locale).toUtf8().constData();
9+ const QByteArray qkey = keyTemplate.arg(locale).toUtf8();
10+ const char* key = qkey.constData();
11 if (g_desktop_app_info_has_key(info, key)) {
12 return d->getKey(key);
13 }

Subscribers

People subscribed via source and target branches