Re: Review Request 116087: KCrash: remove usage of strlcpy
> On March 1, 2014, 4:17 p.m., David Faure wrote:
> > Hmm, this might be equivalent, but all it means is that the orig code was
> > wrong.
> >
> > We should not make any memory allocations within the crash handler.
> >
> > So we should instead store the startup id as a const char* somewhere and
> > use strlcpy.
> >
> > Unless we can make sure that the call to startupId() will always just
> > return an existing QByteArray - but looking at the code, this doesn't seem
> > to be the case.
>
> Alex Merry wrote:
> Hrm... I think we're actually querying the wrong place anyway. We should
> be asking the xcb platform plugin, since that's what actually handles the
> startup notifications, since the demise of KApplication (perhaps that part of
> KStartupInfo's functionality should be stripped out?).
>
> Note that the platform plugin does always just return an existing
> QByteArray, so that should be fine. Which is good, because taking our own
> copy outside the handler would not work, as we would need to know when it was
> cleared.
>
> Alex Merry wrote:
> Actually, you don't get access to the QByteArray. You could do something
> like
>
> QPlatformNativeInterface *platform =
> QGuiApplication::platformNativeInterface();
> const char * startupId = reinterpret_cast *>(platform->nativeResourceForIntegration(QByteArrayLiteral("startupid")));
> if (startupId && *startupId) {
> argv[i++] = "--startupid";
> argv[i++] = startupId;
> }
>
> Since we're in the crash handler, is it safe to assume that the rest of
> the application is stopped, and so that string will never disappear from
> underneath us?
>
> Alexander Richardson wrote:
> I'll update the review to use this solution if someone else can confirm
> that this is safe. Even in multithreaded environments? Does the crash handler
> stop all threads?
>
> Kevin Ottens wrote:
> I think so yes, David could you confirm?
I'm not an expert on crash handling in a multithreaded application, but anyway,
threads do not call into the xcb plugin (it's not threadsafe, and it's a GUI
thing anyway). So yeah Alex Merry's suggestion sounds fine.
- David
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/116087/#review51440
---
On March 12, 2014, 3:26 p.m., Alexander Richardson wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116087/
> ---
>
> (Updated March 12, 2014, 3:26 p.m.)
>
>
> Review request for KDE Frameworks and David Faure.
>
>
> Repository: kcrash
>
>
> Description
> ---
>
> remove usage of strlcpy
>
> That copy was actually unnecessary, incrementing the refcount on
> QByteArray and then calling constData() is enough
>
>
> Diffs
> -
>
> src/kcrash.cpp 6c41022206130f186d52ddbb77afd58c429f368f
> src/strlcpy-fake.c 9b2dc68c466908d11370ba0c4bbe8d315962da5d
> src/CMakeLists.txt d19b97f98e057d5537cf0eb6a8e1997d2a24cb1e
> src/config-strlcpy.h.cmake 5d9163d7a60d89b9792afcdf498af6425a9038a8
>
> Diff: https://git.reviewboard.kde.org/r/116087/diff/
>
>
> Testing
> ---
>
> Compiles
>
>
> Thanks,
>
> Alexander Richardson
>
>
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116087: KCrash: remove usage of strlcpy
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116087/ --- (Updated March 12, 2014, 3:26 p.m.) Review request for KDE Frameworks and David Faure. Changes --- Pending question for David. Repository: kcrash Description --- remove usage of strlcpy That copy was actually unnecessary, incrementing the refcount on QByteArray and then calling constData() is enough Diffs - src/kcrash.cpp 6c41022206130f186d52ddbb77afd58c429f368f src/strlcpy-fake.c 9b2dc68c466908d11370ba0c4bbe8d315962da5d src/CMakeLists.txt d19b97f98e057d5537cf0eb6a8e1997d2a24cb1e src/config-strlcpy.h.cmake 5d9163d7a60d89b9792afcdf498af6425a9038a8 Diff: https://git.reviewboard.kde.org/r/116087/diff/ Testing --- Compiles Thanks, Alexander Richardson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116087: KCrash: remove usage of strlcpy
> On March 1, 2014, 4:17 p.m., David Faure wrote:
> > Hmm, this might be equivalent, but all it means is that the orig code was
> > wrong.
> >
> > We should not make any memory allocations within the crash handler.
> >
> > So we should instead store the startup id as a const char* somewhere and
> > use strlcpy.
> >
> > Unless we can make sure that the call to startupId() will always just
> > return an existing QByteArray - but looking at the code, this doesn't seem
> > to be the case.
>
> Alex Merry wrote:
> Hrm... I think we're actually querying the wrong place anyway. We should
> be asking the xcb platform plugin, since that's what actually handles the
> startup notifications, since the demise of KApplication (perhaps that part of
> KStartupInfo's functionality should be stripped out?).
>
> Note that the platform plugin does always just return an existing
> QByteArray, so that should be fine. Which is good, because taking our own
> copy outside the handler would not work, as we would need to know when it was
> cleared.
>
> Alex Merry wrote:
> Actually, you don't get access to the QByteArray. You could do something
> like
>
> QPlatformNativeInterface *platform =
> QGuiApplication::platformNativeInterface();
> const char * startupId = reinterpret_cast *>(platform->nativeResourceForIntegration(QByteArrayLiteral("startupid")));
> if (startupId && *startupId) {
> argv[i++] = "--startupid";
> argv[i++] = startupId;
> }
>
> Since we're in the crash handler, is it safe to assume that the rest of
> the application is stopped, and so that string will never disappear from
> underneath us?
>
> Alexander Richardson wrote:
> I'll update the review to use this solution if someone else can confirm
> that this is safe. Even in multithreaded environments? Does the crash handler
> stop all threads?
I think so yes, David could you confirm?
- Kevin
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/116087/#review51440
---
On Feb. 26, 2014, 5 p.m., Alexander Richardson wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116087/
> ---
>
> (Updated Feb. 26, 2014, 5 p.m.)
>
>
> Review request for KDE Frameworks.
>
>
> Repository: kcrash
>
>
> Description
> ---
>
> remove usage of strlcpy
>
> That copy was actually unnecessary, incrementing the refcount on
> QByteArray and then calling constData() is enough
>
>
> Diffs
> -
>
> src/kcrash.cpp 6c41022206130f186d52ddbb77afd58c429f368f
> src/strlcpy-fake.c 9b2dc68c466908d11370ba0c4bbe8d315962da5d
> src/CMakeLists.txt d19b97f98e057d5537cf0eb6a8e1997d2a24cb1e
> src/config-strlcpy.h.cmake 5d9163d7a60d89b9792afcdf498af6425a9038a8
>
> Diff: https://git.reviewboard.kde.org/r/116087/diff/
>
>
> Testing
> ---
>
> Compiles
>
>
> Thanks,
>
> Alexander Richardson
>
>
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116087: KCrash: remove usage of strlcpy
> On March 1, 2014, 5:17 p.m., David Faure wrote:
> > Hmm, this might be equivalent, but all it means is that the orig code was
> > wrong.
> >
> > We should not make any memory allocations within the crash handler.
> >
> > So we should instead store the startup id as a const char* somewhere and
> > use strlcpy.
> >
> > Unless we can make sure that the call to startupId() will always just
> > return an existing QByteArray - but looking at the code, this doesn't seem
> > to be the case.
>
> Alex Merry wrote:
> Hrm... I think we're actually querying the wrong place anyway. We should
> be asking the xcb platform plugin, since that's what actually handles the
> startup notifications, since the demise of KApplication (perhaps that part of
> KStartupInfo's functionality should be stripped out?).
>
> Note that the platform plugin does always just return an existing
> QByteArray, so that should be fine. Which is good, because taking our own
> copy outside the handler would not work, as we would need to know when it was
> cleared.
>
> Alex Merry wrote:
> Actually, you don't get access to the QByteArray. You could do something
> like
>
> QPlatformNativeInterface *platform =
> QGuiApplication::platformNativeInterface();
> const char * startupId = reinterpret_cast *>(platform->nativeResourceForIntegration(QByteArrayLiteral("startupid")));
> if (startupId && *startupId) {
> argv[i++] = "--startupid";
> argv[i++] = startupId;
> }
>
> Since we're in the crash handler, is it safe to assume that the rest of
> the application is stopped, and so that string will never disappear from
> underneath us?
I'll update the review to use this solution if someone else can confirm that
this is safe. Even in multithreaded environments? Does the crash handler stop
all threads?
- Alexander
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/116087/#review51440
---
On Feb. 26, 2014, 6 p.m., Alexander Richardson wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116087/
> ---
>
> (Updated Feb. 26, 2014, 6 p.m.)
>
>
> Review request for KDE Frameworks.
>
>
> Repository: kcrash
>
>
> Description
> ---
>
> remove usage of strlcpy
>
> That copy was actually unnecessary, incrementing the refcount on
> QByteArray and then calling constData() is enough
>
>
> Diffs
> -
>
> src/kcrash.cpp 6c41022206130f186d52ddbb77afd58c429f368f
> src/strlcpy-fake.c 9b2dc68c466908d11370ba0c4bbe8d315962da5d
> src/CMakeLists.txt d19b97f98e057d5537cf0eb6a8e1997d2a24cb1e
> src/config-strlcpy.h.cmake 5d9163d7a60d89b9792afcdf498af6425a9038a8
>
> Diff: https://git.reviewboard.kde.org/r/116087/diff/
>
>
> Testing
> ---
>
> Compiles
>
>
> Thanks,
>
> Alexander Richardson
>
>
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116087: KCrash: remove usage of strlcpy
> On March 1, 2014, 4:17 p.m., David Faure wrote:
> > Hmm, this might be equivalent, but all it means is that the orig code was
> > wrong.
> >
> > We should not make any memory allocations within the crash handler.
> >
> > So we should instead store the startup id as a const char* somewhere and
> > use strlcpy.
> >
> > Unless we can make sure that the call to startupId() will always just
> > return an existing QByteArray - but looking at the code, this doesn't seem
> > to be the case.
>
> Alex Merry wrote:
> Hrm... I think we're actually querying the wrong place anyway. We should
> be asking the xcb platform plugin, since that's what actually handles the
> startup notifications, since the demise of KApplication (perhaps that part of
> KStartupInfo's functionality should be stripped out?).
>
> Note that the platform plugin does always just return an existing
> QByteArray, so that should be fine. Which is good, because taking our own
> copy outside the handler would not work, as we would need to know when it was
> cleared.
Actually, you don't get access to the QByteArray. You could do something like
QPlatformNativeInterface *platform = QGuiApplication::platformNativeInterface();
const char * startupId = reinterpret_cast(platform->nativeResourceForIntegration(QByteArrayLiteral("startupid")));
if (startupId && *startupId) {
argv[i++] = "--startupid";
argv[i++] = startupId;
}
Since we're in the crash handler, is it safe to assume that the rest of the
application is stopped, and so that string will never disappear from underneath
us?
- Alex
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/116087/#review51440
---
On Feb. 26, 2014, 5 p.m., Alexander Richardson wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116087/
> ---
>
> (Updated Feb. 26, 2014, 5 p.m.)
>
>
> Review request for KDE Frameworks.
>
>
> Repository: kcrash
>
>
> Description
> ---
>
> remove usage of strlcpy
>
> That copy was actually unnecessary, incrementing the refcount on
> QByteArray and then calling constData() is enough
>
>
> Diffs
> -
>
> src/kcrash.cpp 6c41022206130f186d52ddbb77afd58c429f368f
> src/strlcpy-fake.c 9b2dc68c466908d11370ba0c4bbe8d315962da5d
> src/CMakeLists.txt d19b97f98e057d5537cf0eb6a8e1997d2a24cb1e
> src/config-strlcpy.h.cmake 5d9163d7a60d89b9792afcdf498af6425a9038a8
>
> Diff: https://git.reviewboard.kde.org/r/116087/diff/
>
>
> Testing
> ---
>
> Compiles
>
>
> Thanks,
>
> Alexander Richardson
>
>
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116087: KCrash: remove usage of strlcpy
> On March 1, 2014, 4:17 p.m., David Faure wrote: > > Hmm, this might be equivalent, but all it means is that the orig code was > > wrong. > > > > We should not make any memory allocations within the crash handler. > > > > So we should instead store the startup id as a const char* somewhere and > > use strlcpy. > > > > Unless we can make sure that the call to startupId() will always just > > return an existing QByteArray - but looking at the code, this doesn't seem > > to be the case. Hrm... I think we're actually querying the wrong place anyway. We should be asking the xcb platform plugin, since that's what actually handles the startup notifications, since the demise of KApplication (perhaps that part of KStartupInfo's functionality should be stripped out?). Note that the platform plugin does always just return an existing QByteArray, so that should be fine. Which is good, because taking our own copy outside the handler would not work, as we would need to know when it was cleared. - Alex --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116087/#review51440 --- On Feb. 26, 2014, 5 p.m., Alexander Richardson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/116087/ > --- > > (Updated Feb. 26, 2014, 5 p.m.) > > > Review request for KDE Frameworks. > > > Repository: kcrash > > > Description > --- > > remove usage of strlcpy > > That copy was actually unnecessary, incrementing the refcount on > QByteArray and then calling constData() is enough > > > Diffs > - > > src/kcrash.cpp 6c41022206130f186d52ddbb77afd58c429f368f > src/strlcpy-fake.c 9b2dc68c466908d11370ba0c4bbe8d315962da5d > src/CMakeLists.txt d19b97f98e057d5537cf0eb6a8e1997d2a24cb1e > src/config-strlcpy.h.cmake 5d9163d7a60d89b9792afcdf498af6425a9038a8 > > Diff: https://git.reviewboard.kde.org/r/116087/diff/ > > > Testing > --- > > Compiles > > > Thanks, > > Alexander Richardson > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116087: KCrash: remove usage of strlcpy
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116087/#review51440 --- Hmm, this might be equivalent, but all it means is that the orig code was wrong. We should not make any memory allocations within the crash handler. So we should instead store the startup id as a const char* somewhere and use strlcpy. Unless we can make sure that the call to startupId() will always just return an existing QByteArray - but looking at the code, this doesn't seem to be the case. - David Faure On Feb. 26, 2014, 5 p.m., Alexander Richardson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/116087/ > --- > > (Updated Feb. 26, 2014, 5 p.m.) > > > Review request for KDE Frameworks. > > > Repository: kcrash > > > Description > --- > > remove usage of strlcpy > > That copy was actually unnecessary, incrementing the refcount on > QByteArray and then calling constData() is enough > > > Diffs > - > > src/kcrash.cpp 6c41022206130f186d52ddbb77afd58c429f368f > src/strlcpy-fake.c 9b2dc68c466908d11370ba0c4bbe8d315962da5d > src/CMakeLists.txt d19b97f98e057d5537cf0eb6a8e1997d2a24cb1e > src/config-strlcpy.h.cmake 5d9163d7a60d89b9792afcdf498af6425a9038a8 > > Diff: https://git.reviewboard.kde.org/r/116087/diff/ > > > Testing > --- > > Compiles > > > Thanks, > > Alexander Richardson > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116087: KCrash: remove usage of strlcpy
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116087/#review51008 --- Ship it! The QByteArray clearly has the same lifetime as the argv array itself, so should be fine. - Alex Merry On Feb. 26, 2014, 5 p.m., Alexander Richardson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/116087/ > --- > > (Updated Feb. 26, 2014, 5 p.m.) > > > Review request for KDE Frameworks. > > > Repository: kcrash > > > Description > --- > > remove usage of strlcpy > > That copy was actually unnecessary, incrementing the refcount on > QByteArray and then calling constData() is enough > > > Diffs > - > > src/kcrash.cpp 6c41022206130f186d52ddbb77afd58c429f368f > src/strlcpy-fake.c 9b2dc68c466908d11370ba0c4bbe8d315962da5d > src/CMakeLists.txt d19b97f98e057d5537cf0eb6a8e1997d2a24cb1e > src/config-strlcpy.h.cmake 5d9163d7a60d89b9792afcdf498af6425a9038a8 > > Diff: https://git.reviewboard.kde.org/r/116087/diff/ > > > Testing > --- > > Compiles > > > Thanks, > > Alexander Richardson > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 116087: KCrash: remove usage of strlcpy
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116087/ --- Review request for KDE Frameworks. Repository: kcrash Description --- remove usage of strlcpy That copy was actually unnecessary, incrementing the refcount on QByteArray and then calling constData() is enough Diffs - src/kcrash.cpp 6c41022206130f186d52ddbb77afd58c429f368f src/strlcpy-fake.c 9b2dc68c466908d11370ba0c4bbe8d315962da5d src/CMakeLists.txt d19b97f98e057d5537cf0eb6a8e1997d2a24cb1e src/config-strlcpy.h.cmake 5d9163d7a60d89b9792afcdf498af6425a9038a8 Diff: https://git.reviewboard.kde.org/r/116087/diff/ Testing --- Compiles Thanks, Alexander Richardson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
