D28535: [KIO-MTP] Fix null pointer dereference

This revision was automatically updated to reflect the committed changes. Closed by commit R320:94e7b64325f9: [KIO-MTP] Fix null pointer dereference (authored by feverfew). REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28535?vs=79252=79998 REVISION

D28535: [KIO-MTP] Fix null pointer dereference

elvisangelaccio added a comment. In D28535#642298 , @feverfew wrote: > In D28535#642289 , @elvisangelaccio wrote: > > > In D28535#640833 , @fvogt wrote:

D28535: [KIO-MTP] Fix null pointer dereference

feverfew added a comment. In D28535#642298 , @feverfew wrote: > In D28535#642289 , @elvisangelaccio wrote: > > > In D28535#640833 , @fvogt wrote: > >

D28535: [KIO-MTP] Fix null pointer dereference

feverfew added a comment. In D28535#642289 , @elvisangelaccio wrote: > In D28535#640833 , @fvogt wrote: > > > I assume there is a reason why `MTPDevice::getDevice()` has code for handling this very

D28535: [KIO-MTP] Fix null pointer dereference

anthonyfieroni added a comment. @elvisangelaccio this peace of code is purely wrong at least `m_storages` is not updated to new device and not only. This code should never exists or try to hide some other bug. REPOSITORY R320 KIO Extras BRANCH fixNullPtr (branched from master)

D28535: [KIO-MTP] Fix null pointer dereference

elvisangelaccio accepted this revision. elvisangelaccio added a comment. This revision is now accepted and ready to land. In D28535#640833 , @fvogt wrote: > I assume there is a reason why `MTPDevice::getDevice()` has code for handling this very

D28535: [KIO-MTP] Fix null pointer dereference

anthonyfieroni added a comment. Ok that's look good to me. We can move that code in constructor just before loop for storages, then use m_mtpdevice instead of raw device, but it looks like removed code is just a noise and it shouldn't present at all. +1 REPOSITORY R320 KIO Extras

D28535: [KIO-MTP] Fix null pointer dereference

feverfew added a comment. Ok so I've updated the code as can be seen. Mucked around with disconnecting/connecting, no issues on my side at least. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28535 To: feverfew, akrutzler, dfaure, elvisangelaccio Cc:

D28535: [KIO-MTP] Fix null pointer dereference

feverfew updated this revision to Diff 79252. feverfew added a comment. - Don't try to release device in get method REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28535?vs=79198=79252 BRANCH fixNullPtr (branched from master) REVISION DETAIL

D28535: [KIO-MTP] Fix null pointer dereference

fvogt added a comment. I assume there is a reason why `MTPDevice::getDevice()` has code for handling this very specific case, so I wouldn't just remove it without figuring out why: ../https://i.redd.it/hfnl7xo8yovy.gif If not, that would indeed be the best option. REPOSITORY R320 KIO

D28535: [KIO-MTP] Fix null pointer dereference

feverfew added a comment. In D28535#640703 , @anthonyfieroni wrote: > In D28535#640699 , @feverfew wrote: > > > So to be succinct, the only correct fix here is to change `getDevice()` to `return

D28535: [KIO-MTP] Fix null pointer dereference

anthonyfieroni added a comment. In D28535#640699 , @feverfew wrote: > So to be succinct, the only correct fix here is to change `getDevice()` to `return m_mtpdevice`? Yes, then check if it's crash, in all other place `LIBMTP_xxx` should

D28535: [KIO-MTP] Fix null pointer dereference

feverfew added a comment. If you want to, feel free, I'm a bit tight on time. But I will say this. the whole `getDevice()` function confuses me. I'm not entirely sure why this `if` check is necessary at all. The lifetime of devices and its children (i.e storage) should be managed by the

D28535: [KIO-MTP] Fix null pointer dereference

anthonyfieroni added a comment. @feverfew are you gonna try what i'm writing about or i should do it? Just use cached device, do not reopen since it'll return nullptr. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D28535 To: feverfew, akrutzler, dfaure,

D28535: [KIO-MTP] Fix null pointer dereference

fvogt added a comment. In D28535#640682 , @feverfew wrote: > In D28535#640674 , @fvogt wrote: > > > What you're suggesting is to change `MTPDevice::getDevice` to return the old device if reopening

D28535: [KIO-MTP] Fix null pointer dereference

feverfew added a comment. In D28535#640674 , @fvogt wrote: > What you're suggesting is to change `MTPDevice::getDevice` to return the old device if reopening fails - but reopening without releasing might not work. This seems to be a

D28535: [KIO-MTP] Fix null pointer dereference

fvogt added a comment. In D28535#640680 , @anthonyfieroni wrote: > I see we don't speak in same language :) > `LIBMTP_Open_Raw_Device_Uncached(_rawdevice);` > returns nullptr that's normal since device is inaccessible, i mean it does not

D28535: [KIO-MTP] Fix null pointer dereference

anthonyfieroni added a comment. I see we don't speak in same language :) `LIBMTP_Open_Raw_Device_Uncached(_rawdevice);` returns nullptr that's normal since device is inaccessible, i mean it does not need to call `LIBMTP_Release_Device` using `m_mtpdevice` is safe it's not nullptr, it's

D28535: [KIO-MTP] Fix null pointer dereference

fvogt added a comment. There is no such thing as an "invalid device" at that point anymore. There's only nullptr. LIBMTP_mtpdevice_t *MTPDevice::getDevice() { if (!m_mtpdevice->storage) { qCDebug(LOG_KIOD_KMTPD) << "no storage found: reopen mtpdevice";

D28535: [KIO-MTP] Fix null pointer dereference

anthonyfieroni added a comment. In D28535#640672 , @fvogt wrote: > If `getDevice()` returns nullptr, this means that `MTPDevice::getDevice()` returns nullptr. This can only happen if `m_mtpdevice` is nullptr, which will crash in

D28535: [KIO-MTP] Fix null pointer dereference

fvogt added a comment. In D28535#640656 , @anthonyfieroni wrote: > You're right about bug report, but it can fail in any other place, just in particular version it happen in `updateStorageInfo` Can we cache `getDevice` in m_device (in

D28535: [KIO-MTP] Fix null pointer dereference

anthonyfieroni added a comment. You're right about bug report, but it can fail in any other place, just in particular version it happen in `updateStorageInfo` Can we cache `getDevice` in m_device (in constructor) then use it everywhere. I think libmtp has guard against disconnected device

D28535: [KIO-MTP] Fix null pointer dereference

feverfew created this revision. feverfew added reviewers: akrutzler, dfaure, elvisangelaccio. Herald added projects: Dolphin, Frameworks. Herald added subscribers: kfm-devel, kde-frameworks-devel. feverfew requested review of this revision. REVISION SUMMARY A null pointer can be returned from