Re: Review Request 127968: kshareddatacache: Fix invalid use of & to avoid unaligned reads

2016-05-20 Thread Fabian Vogt
-mail. To reply, visit: https://git.reviewboard.kde.org/r/127968/#review95630 ------- On Mai 19, 2016, 6:13 nachm., Fabian Vogt wrote: > > --- > This is an automatically generated e

Review Request 127968: kshareddatacache: Fix invalid use of & to avoid unaligned reads

2016-05-19 Thread Fabian Vogt
t change the returned values on platforms where unaligned reads are permitted, so I didn't bump the version. Diffs - src/lib/caching/kshareddatacache.cpp 50bbf64 Diff: https://git.reviewboard.kde.org/r/127968/diff/ Testing --- Ran make test, all passed. Thanks, Fab

Re: Review Request 127968: kshareddatacache: Fix invalid use of & to avoid unaligned reads

2016-05-20 Thread Fabian Vogt
marked as submitted. Review request for KDE Frameworks, David Faure and Michael Pyne. Changes --- Submitted with commit 2bd185e719a83143e140758b0fad6d4e1488bc24 by Luca Beltrame on behalf of Fabian Vogt to branch master. Repository: kcoreaddons Description --- That the & is w

Trusting .desktop files

2017-02-10 Thread Fabian Vogt
Hi, The reddit post "How to easily trick $FILE_MANAGER users to execute arbitrary code" (https://www.reddit.com/r/linux/comments/5r6va0) spawned a discussion about .desktop files. This is normally just a minor security issue as it requires manual user interaction. However, Plasma's

D5328: Ignore warnings during appdata generation

2017-04-07 Thread Fabian Vogt
fvogt created this revision. Restricted Application added a project: Frameworks. REVISION SUMMARY The Plasma/PopupApplet service type for example got deprecated and removed, but is still used by various parts of Plasma 5.8. This causes kpackagetool to emit warnings, but it still succeeds.

D5328: Ignore warnings during appdata generation

2017-04-07 Thread Fabian Vogt
This revision was automatically updated to reflect the committed changes. Closed by commit R290:a0da2bd735d0: Ignore warnings during appdata generation (authored by fvogt). REPOSITORY R290 KPackage CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5328?vs=13175=13176 REVISION DETAIL

D5312: KPasswordDialog: don't hide visibility action in plaintext mode

2017-04-06 Thread Fabian Vogt
fvogt accepted this revision. fvogt added a comment. This revision is now accepted and ready to land. Tested locally, works fine! The behaviour is a bit unexpected, but IMO correct: - Enter a password - Make it visible - Delete it - Toggle visibility -> VisibilityAction

D4847: KAuth integration in document saving

2017-04-10 Thread Fabian Vogt
fvogt added a comment. Adding to Luca's comment, I also find two additional issues with this approach, it is absolutely impossible to make this secure. There is always a race condition between acquiring the privilege and renaming the file to the new location. Only solution for that is to

D4847: KAuth integration in document saving

2017-04-10 Thread Fabian Vogt
fvogt added a comment. In https://phabricator.kde.org/D4847#101088, @ivan wrote: > @lbeltrame > > I don't really see 'reasoning' in that report more than something that sounds like 'polkit is a hack to replace suid for setting the system time and time zone'. Me neither. In

D5394: KAuth integration in document saving - vol. 2

2017-04-11 Thread Fabian Vogt
fvogt added a comment. Looks good! > should I make a checksum of the data before sending to KAuth action and then match it with checksum sent back by the action? Yes, that way you could also implement a way to show the checksum to the user. Make sure to avoid any TOCTTOU issues.

D5394: KAuth integration in document saving - vol. 2

2017-04-11 Thread Fabian Vogt
fvogt requested changes to this revision. fvogt added a comment. This revision now requires changes to proceed. In https://phabricator.kde.org/D5394#101291, @aacid wrote: > In https://phabricator.kde.org/D5394#101275, @fvogt wrote: > > > what are the permissions of the temporary file

D5394: KAuth integration in document saving - vol. 2

2017-04-15 Thread Fabian Vogt
fvogt added a comment. It is currently not possible to avoid a race condition as QSaveFile is broken and use of it is currently insecure as the file is created world-readable. It can be avoided by placing the file in a drwx-- directory, but that's more work than just using

D5394: KAuth integration in document saving - vol. 2

2017-04-16 Thread Fabian Vogt
fvogt added a comment. In https://phabricator.kde.org/D5394#102605, @dfaure wrote: > Would it help if QSaveFile had an API to set more restrictive permissions on the temp file? No, the fix is simple: Revert

D5394: KAuth integration in document saving - vol. 2

2017-04-16 Thread Fabian Vogt
fvogt requested changes to this revision. fvogt added a comment. This revision now requires changes to proceed. Thanks! So far I only found two issues (see comments). Apart from that it would be great to see the application name and the target/source file in the polkit dialog, but I

D5394: KAuth integration in document saving - vol. 2

2017-04-18 Thread Fabian Vogt
fvogt added a comment. Thanks! Only one issue remaining (I somehow missed during the last time): Directory rename race. Someone with the appropriate permissions (unlikely) could change the directory the path refers to in between the verification and the chmod/fchown/rename.

D5394: KAuth integration in document saving - vol. 2

2017-04-20 Thread Fabian Vogt
fvogt added a comment. Thanks again! The changes look good, but strace shows that the QFile operation still use absolute paths, apparently it resolves the "." manually with getcwd: chdir("/tmp") = 0 stat("/tmp/permfile", {st_mode=S_IFREG|0644, st_size=5,

D4937: Add KFileWidget::setSelectedUrl()

2017-03-08 Thread Fabian Vogt
fvogt accepted this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D4937 To: dfaure, fvogt Cc: fvogt, #frameworks

D4937: Add KFileWidget::setSelectedUrl()

2017-03-07 Thread Fabian Vogt
fvogt requested changes to this revision. fvogt added a comment. This revision now requires changes to proceed. Yes, but constructing a relative QUrl isn't really that easy to do (definitely not obvious at that) and so future users will either pass a filename with QUrl(filename) (broken) or

D4937: Add KFileWidget::setSelectedUrl()

2017-03-07 Thread Fabian Vogt
fvogt accepted this revision. fvogt added a comment. This revision is now accepted and ready to land. Looks good to me, although a new setSelectedFilename method would be nice to have REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D4937 To: dfaure,

D4937: Add KFileWidget::setSelectedUrl()

2017-03-04 Thread Fabian Vogt
fvogt requested changes to this revision. fvogt added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > kfilewidget.h:150 > */ > void setSelection(const QString ); > This function only works correctly for URLs, as filenames can contain ':'s. However,

D4937: Add KFileWidget::setSelectedUrl()

2017-03-05 Thread Fabian Vogt
fvogt added inline comments. INLINE COMMENTS > dfaure wrote in kfilewidget.h:150 > Very good point. > > But there might be other users of this class who need the ability to select > by filename. > I think this means we have two options: > > - changing setSelection to only handle filenames,

D4937: Add KFileWidget::setSelectedUrl()

2017-03-05 Thread Fabian Vogt
fvogt added inline comments. INLINE COMMENTS > dfaure wrote in kfilewidget.h:150 > I guess you mean "with setSelection". > I made setSelectedUrl handle only full URLs. No, I meant setSelectedUrl indeed. It works for relative URLs as well. Both the function itself and setLocationText check

D7318: KFileItemDelegate: Always reserve space for icons

2017-08-15 Thread Fabian Vogt
fvogt created this revision. Restricted Application added a project: Frameworks. REVISION SUMMARY Even null icons are drawn so also reserve space for them to avoid overlapping. TEST PLAN http://imgur.com/a/fG2T0 REPOSITORY R241 KIO BRANCH iconfix-master REVISION DETAIL

D7318: KFileItemDelegate: Always reserve space for icons

2017-08-15 Thread Fabian Vogt
fvogt added a comment. It would be possible to avoid drawing null QIcons as well, but that would result in broken alignment. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D7318 To: fvogt, #frameworks Cc: #frameworks

D7318: KFileItemDelegate: Always reserve space for icons

2017-08-15 Thread Fabian Vogt
fvogt added a comment. The underlying issue is that KIconLoader's implementation of isNull (in virtual_hook) is not really meaningful. In cases where a name for a nonexistent icon is passed, pixmap will draw the "unknown" icon, but isNull still returns true. So the question is whether

D7250: Avoid some unnecessary theme content lookups

2017-08-11 Thread Fabian Vogt
fvogt closed this revision. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D7250 To: fvogt, #plasma, mart Cc: #frameworks

D7250: Avoid some unnecessary theme content lookups

2017-08-11 Thread Fabian Vogt
fvogt updated this revision to Diff 18004. fvogt added a comment. Forgot a check REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7250?vs=18003=18004 BRANCH master REVISION DETAIL https://phabricator.kde.org/D7250 AFFECTED FILES

D7250: Avoid some unnecessary theme content lookups

2017-08-11 Thread Fabian Vogt
fvogt added a reviewer: Plasma. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D7250 To: fvogt, #plasma Cc: #frameworks

D7250: Avoid some unnecessary theme content lookups

2017-08-11 Thread Fabian Vogt
fvogt created this revision. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. REVISION SUMMARY imagePath can be an absolute path into an iconTheme -> Do not try to find it in the Plasma theme imagePath can be empty -> Do not try

D6679: Treat Button/ToolButton labels as plaintext

2017-07-13 Thread Fabian Vogt
fvogt created this revision. Restricted Application added projects: Plasma, Frameworks. Restricted Application added a subscriber: Frameworks. REVISION SUMMARY The label text gets treated as RichText/StyledText, which is required to display mnemonics underlined. Therefore it is necessary to

D6679: Treat Button/ToolButton labels as plaintext

2017-07-13 Thread Fabian Vogt
fvogt updated this revision to Diff 16649. fvogt added a comment. Fix typo in ToolButtonStyle REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6679?vs=16648=16649 BRANCH master REVISION DETAIL https://phabricator.kde.org/D6679

D6679: Treat Button/ToolButton labels as plaintext

2017-07-13 Thread Fabian Vogt
fvogt marked an inline comment as done. fvogt added inline comments. INLINE COMMENTS > broulik wrote in ToolButtonStyle.qml:100 > `Text.StyledText` I think the priority queue in my brain is somehow backwards REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL

D6543: [KPageListViewDelegate] Pass widget to drawPrimitive

2017-07-07 Thread Fabian Vogt
fvogt accepted this revision. fvogt added a comment. This revision is now accepted and ready to land. Tested, works fine here. I don't see any other cases of drawPrimitive being invoked without passing a widget. REPOSITORY R236 KWidgetsAddons REVISION DETAIL

D6679: Treat Button/ToolButton labels as plaintext

2017-07-25 Thread Fabian Vogt
This revision was automatically updated to reflect the committed changes. fvogt marked an inline comment as done. Closed by commit R242:48a8245db4d8: Treat Button/ToolButton labels as plaintext (authored by fvogt). REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE

D5394: KAuth integration in document saving - vol. 2

2017-04-30 Thread Fabian Vogt
fvogt accepted this revision. fvogt added a comment. This revision is now accepted and ready to land. The strace snipped you posted looks good to me! That new files are created with 0644 permissions is fine, the default umask is 022 anyway so running "echo test > file" as root would

D6002: Identify PIE binaries (application/x-sharedlib) as executable files

2017-05-28 Thread Fabian Vogt
fvogt created this revision. Restricted Application added a project: Frameworks. REVISION SUMMARY x86_64 binaries compiled with PIE are just shared objects with the executable bit set. Without this patch, kio does not know that they can be executed as well, causing "kioclient5 exec" to ask

D6002: Identify PIE binaries (application/x-sharedlib) as executable files

2017-05-29 Thread Fabian Vogt
This revision was automatically updated to reflect the committed changes. Closed by commit R241:4122b52fee54: Identify PIE binaries (application/x-sharedlib) as executable files (authored by fvogt). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE

D6002: Identify PIE binaries (application/x-sharedlib) as executable files

2017-05-29 Thread Fabian Vogt
fvogt added a comment. In https://phabricator.kde.org/D6002#112526, @dfaure wrote: > Then isExecutable() is wrong API, it misses the check for the executable bit. But that's non trivial to fix (check if callers would have the info, deprecate method and add replacement which takes more

D6002: Identify PIE binaries (application/x-sharedlib) as executable files

2017-05-29 Thread Fabian Vogt
fvogt updated this revision to Diff 14928. fvogt added a comment. Add a comment referencing the fdo bug report REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6002?vs=14914=14928 BRANCH master REVISION DETAIL https://phabricator.kde.org/D6002 AFFECTED

D6002: Identify PIE binaries (application/x-sharedlib) as executable files

2017-05-29 Thread Fabian Vogt
fvogt added a comment. In https://phabricator.kde.org/D6002#112407, @dfaure wrote: > This is a workaround for https://bugs.freedesktop.org/show_bug.cgi?id=97226 but indeed, there seems to be no other way. > > Maybe add a link to that bug report in one of the uses of x-sharedlib in

D7318: KFileItemDelegate: Always reserve space for icons

2017-09-16 Thread Fabian Vogt
fvogt added a comment. Ping #2. If no reply until monday I'll assume this is ok. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D7318 To: fvogt, #frameworks Cc: cfeck, #frameworks

D7318: KFileItemDelegate: Always reserve space for icons

2017-09-19 Thread Fabian Vogt
This revision was automatically updated to reflect the committed changes. Closed by commit R241:31361267d419: KFileItemDelegate: Always reserve space for icons (authored by fvogt). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7318?vs=18165=19665 REVISION

D7929: [WIP] Add new Column View option to KDirOperator

2017-09-21 Thread Fabian Vogt
fvogt added a comment. In https://phabricator.kde.org/D7929#147742, @elvisangelaccio wrote: > I'm not sure if it's a good idea to fork QColumnView in KIO. Is it not possible to contribute the changes upstream? It's not that easy - QColumnView is practically abandoned upstream

D7929: [WIP] Add new Column View option to KDirOperator

2017-09-22 Thread Fabian Vogt
fvogt added a comment. In https://phabricator.kde.org/D7929#147784, @ngraham wrote: > I'll just leave this here: ;-) https://bugs.kde.org/show_bug.cgi?id=290747 In https://phabricator.kde.org/D7929#147806, @dhaumann wrote: > Since this adds the column view again, how

D7929: [WIP] Add new Column View option to KDirOperator

2017-09-21 Thread Fabian Vogt
fvogt created this revision. fvogt added a reviewer: Frameworks. Restricted Application added a project: Frameworks. REVISION SUMMARY It uses an imported and improved version of QColumnView for display. KFileItemDelegate got amended with an arrow if used inside KColumnView, to replace the

D8077: Fix org.kde.plasma.calendar with Qt 5.10

2017-10-05 Thread Fabian Vogt
fvogt added a comment. In https://phabricator.kde.org/D8077#152371, @broulik wrote: > Despite just having built p-f from git I still get > > > Error loading QML file: file:///usr/share/plasma/plasmoids/org.kde.plasma.fuzzyclock/contents/ui/main.qml:51:34: Type

D8070: Fix build with Qt 5.10

2017-09-30 Thread Fabian Vogt
fvogt created this revision. fvogt added a reviewer: Frameworks. Restricted Application added a project: Frameworks. REVISION SUMMARY QByteArray does not include QString anymore, but QString is used in k_pty.h TEST PLAN Did not build before, now does. REPOSITORY R291 KPty BRANCH master

D8069: Fix build with Qt 5.10

2017-09-30 Thread Fabian Vogt
fvogt created this revision. fvogt added reviewers: Frameworks, graesslin. Restricted Application added a project: Frameworks. REVISION SUMMARY QByteArray does not include QString anymore, but that is needed in kxerrorhandler.cpp due to usage of operator overloads. TEST PLAN Did not build

D8069: Fix build with Qt 5.10

2017-09-30 Thread Fabian Vogt
This revision was automatically updated to reflect the committed changes. Closed by commit R278:ec6eec156604: Fix build with Qt 5.10 (authored by fvogt). REPOSITORY R278 KWindowSystem CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8069?vs=20144=20150 REVISION DETAIL

D8070: Fix build with Qt 5.10

2017-09-30 Thread Fabian Vogt
This revision was automatically updated to reflect the committed changes. Closed by commit R291:baad91bc2c45: Fix build with Qt 5.10 (authored by fvogt). REPOSITORY R291 KPty CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8070?vs=20145=20151 REVISION DETAIL

D8077: Fix org.kde.plasma.calendar with Qt 5.10

2017-09-30 Thread Fabian Vogt
fvogt created this revision. fvogt added a reviewer: Plasma. Restricted Application added projects: Plasma, Frameworks. Restricted Application added a subscriber: Frameworks. REVISION SUMMARY Module internal types are not implicitly available anymore simply by placing them inside the module's

D8077: Fix org.kde.plasma.calendar with Qt 5.10

2017-10-01 Thread Fabian Vogt
fvogt added a comment. In https://phabricator.kde.org/D8077#151362, @broulik wrote: > Meh. > > I suspect there are other modules affected as well? I'm not sure, I didn't test anything beyond the default plasma configuration. A quick scan with a script showed mostly false

D8077: Fix org.kde.plasma.calendar with Qt 5.10

2017-10-01 Thread Fabian Vogt
This revision was automatically updated to reflect the committed changes. Closed by commit R242:4f43c236e34e: Fix org.kde.plasma.calendar with Qt 5.10 (authored by fvogt). REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8077?vs=20165=20213

D7929: [WIP] Add new Column View option to KDirOperator

2017-09-24 Thread Fabian Vogt
fvogt added a comment. In https://phabricator.kde.org/D7929#148679, @dhaumann wrote: > Ok. What about licensing? The copied Qt code says LGPLv3 only, while the kio frameworks uses LGPLv2 (and maybe later). Those two licenses are incompatible - we cannot mix LGPLv3 only with an LGPLv2+

D7318: KFileItemDelegate: Always reserve space for icons

2017-08-22 Thread Fabian Vogt
fvogt added a comment. Ping? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D7318 To: fvogt, #frameworks Cc: cfeck, #frameworks

D6665: Make kssl compile against OpenSSL 1.1.0

2017-08-31 Thread Fabian Vogt
fvogt added a comment. I added this patch to the openSUSE packages for testing, it seems to work fine so far, with both openSSL 1.1 and 1.0. I only tested kcm_ssl though, I'm not aware of anything else using it... REPOSITORY R239 KDELibs4Support REVISION DETAIL

D9070: KDE platform plugin: don't force default stylename on user-specified fonts

2017-11-30 Thread Fabian Vogt
fvogt added a comment. In https://phabricator.kde.org/D9070#173835, @rjvbb wrote: > > IMO that's a feature though and is the expected behaviour. For instance, if we change the default window title to be bold, users with "windowTitle=Comic Sans" will also have a bold title. > > So

D7929: [WIP] Add new Column View option to KDirOperator

2017-12-02 Thread Fabian Vogt
fvogt added a comment. In https://phabricator.kde.org/D7929#174572, @dfaure wrote: > That would be the kitemviews framework. > > But really, fixing QColumnView in Qt is the much preferred way to go. > It sure sounds harder right now, but think longer term... > In the past, any

D9446: WIP: Allow to autogenerate and install categories file

2017-12-21 Thread Fabian Vogt
fvogt added a comment. `${KDE_INSTALL_CONFDIR}` should be changed to somewhere else (but maybe in a different patch) IMO `file(APPEND ...)` is the wrong approach, it should collect all categories in a variable and only write it out once at the end (with configure_file). I'm not fluent

D13535: Do not cancel old clipboard selection if it is same as the new one.

2018-06-18 Thread Fabian Vogt
fvogt updated this revision to Diff 36286. fvogt added a comment. - Add unittest. REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13535?vs=36154=36286 BRANCH master REVISION DETAIL https://phabricator.kde.org/D13535 AFFECTED FILES

D13535: Do not cancel old clipboard selection if it is same as the new one.

2018-06-14 Thread Fabian Vogt
fvogt created this revision. fvogt added reviewers: Plasma, Frameworks. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: kde-frameworks-devel. fvogt requested review of this revision. REVISION SUMMARY GTK applications seem to call

D13535: Do not cancel old clipboard selection if it is same as the new one.

2018-06-14 Thread Fabian Vogt
fvogt added a comment. Yes, a unit test makes sense. I tried to add // replace the data source with itself, ensure that it did not get cancelled // replace the data source with itself, ensure that it did not get cancelled selectionOfferedSpy.clear();

D9070: KDE platform plugin: don't force default stylename on user-specified fonts

2018-06-12 Thread Fabian Vogt
fvogt added a comment. In D9070#277428 , @bkchr wrote: > Hi, > this patch is still no applied, would someone do it? :) > Because it already missed 5.13. I pushed it now. REPOSITORY R135 Integration for Qt applications in Plasma

D9070: KDE platform plugin: don't force default stylename on user-specified fonts

2018-06-12 Thread Fabian Vogt
This revision was automatically updated to reflect the committed changes. Closed by commit R135:2e971be576d2: KDE platform plugin: dont force default stylename on user-specified fonts (authored by rjvbb, committed by fvogt). REPOSITORY R135 Integration for Qt applications in Plasma CHANGES

D13222: Use QDateTime for interfacing with QML

2018-05-30 Thread Fabian Vogt
fvogt created this revision. fvogt added reviewers: Plasma, Frameworks. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: kde-frameworks-devel. fvogt requested review of this revision. REVISION SUMMARY QDate from/to JS Date has unexpected results, so

D13222: Use QDateTime for interfacing with QML

2018-06-04 Thread Fabian Vogt
fvogt added a comment. In D13222#273334 , @ngraham wrote: > If the behavior changed in Qt 5.11, does the new code need a Qt version check, or is the patch backwards-compatible with earlier Qt versions? The latter, as it says in the test

D13222: Use QDateTime for interfacing with QML

2018-06-04 Thread Fabian Vogt
This revision was automatically updated to reflect the committed changes. Closed by commit R242:d15f0fa8322d: Use QDateTime for interfacing with QML (authored by fvogt). REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13222?vs=35217=35509

D13377: Fixup @since for skip switcher API

2018-06-06 Thread Fabian Vogt
This revision was automatically updated to reflect the committed changes. Closed by commit R127:46f333a06508: Fixup @since for skip switcher API (authored by fvogt). REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13377?vs=35663=35664 REVISION DETAIL

D13377: Fixup @since for skip switcher API

2018-06-06 Thread Fabian Vogt
fvogt created this revision. fvogt added a reviewer: Plasma. Restricted Application added a project: Frameworks. Restricted Application edited subscribers, added: kde-frameworks-devel; removed: Frameworks. fvogt requested review of this revision. REVISION SUMMARY Not released yet. TEST PLAN

D13128: Make sure ungrab keyboard request is processed before emitting shortcut

2018-05-30 Thread Fabian Vogt
fvogt added a comment. @sontolbonggol Do you have access for pushing this or do you want someone else to do it? REPOSITORY R268 KGlobalAccel REVISION DETAIL https://phabricator.kde.org/D13128 To: sontolbonggol, #frameworks, #plasma, mck182, davidedmundson Cc: davidedmundson, fvogt,

D13752: Kill solidautoeject

2018-06-27 Thread Fabian Vogt
fvogt added a comment. Umount on eject press still works even without the kded module loaded. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D13752 To: broulik, #plasma, #frameworks, adridg, davidedmundson, dfaure, fvogt, ervin Cc: plasma-devel, ragreen,

D9966: [KIO] Fix issues with sharing of file descriptor

2018-01-18 Thread Fabian Vogt
fvogt added a comment. Thanks for the quick reaction! > 1.Fixes buffer overflow due to strcpy. Looks good, but I would prefer an exception or abort instead of silent truncation. Also note that this makes it possible to delete an arbitrary file on non-linux platforms if `path`

D9863: Fix overlap of the first item in KFilePlacesView

2018-01-16 Thread Fabian Vogt
This revision was automatically updated to reflect the committed changes. Closed by commit R241:95f6e4929333: Fix overlap of the first item in KFilePlacesView (authored by fvogt). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D9863?vs=25277=25493#toc REPOSITORY R241 KIO CHANGES SINCE

D9983: Don't stat(/etc/localtime) between read() and write() copying files

2018-01-22 Thread Fabian Vogt
fvogt added a comment. Just a quick idea, it might be wrong: If you use a `QTimer` instead of `QElapsedTimer`, you can get rid of `nextTimeoutMsecs`. This would also simplify the logic a bit. INLINE COMMENTS > jtamate wrote in slavebase.cpp:279 > Reading the documentation

D9983: Don't stat(/etc/localtime) between read() and write() copying files

2018-01-22 Thread Fabian Vogt
fvogt requested changes to this revision. fvogt added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > slavebase.cpp:279 > QByteArray data = d->timeoutData; > -d->nextTimeout = QDateTime(); > +d->nextTimeout.start(); >

D9966: [KIO] Fix issues with sharing of file descriptor

2018-01-24 Thread Fabian Vogt
fvogt requested changes to this revision. fvogt added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > chinmoyr wrote in file_unix.cpp:91 > It is the only case for which this hack seems necessary. For all other cases > a library call (to perform a file

D9863: Fix overlap of the first item in KFilePlacesView

2018-01-13 Thread Fabian Vogt
fvogt created this revision. fvogt added reviewers: Frameworks, dfaure, renatoo. Restricted Application added a project: Frameworks. fvogt requested review of this revision. REVISION SUMMARY The first item overlapped both the section header and the item below. This was caused by taking the

D9844: Don't stat(/etc/localtime) between read() and write() copying files

2018-01-15 Thread Fabian Vogt
fvogt reopened this revision. fvogt added a comment. This revision is now accepted and ready to land. Reverted in master as this broke various important ioslaves, like desktop and trash. I believe the issue is that the timeout fires immediately after construction, without being

[kio] src/core: Revert "Don't stat(/etc/localtime) between read() and write() copying files"

2018-01-15 Thread Fabian Vogt
Git commit 16b21e7223f5bc070dcfd540ede55e5837ccc097 by Fabian Vogt. Committed on 15/01/2018 at 11:31. Pushed by fvogt into branch 'master'. Revert "Don't stat(/etc/localtime) between read() and write() copying files" This reverts commit f7e00b40a6d35fbfe536cdb99d6f22b643676e5e. Th

D9966: [KIO] Fix issues with sharing of file descriptor

2018-01-25 Thread Fabian Vogt
fvogt requested changes to this revision. fvogt added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > chinmoyr wrote in file_unix.cpp:117 > @fvogt I think the errno assignment should be fine here? > Except this one case, the function call whose errno value we

D9830: Fix KFilePreviewGenerator::LayoutBlocker

2018-01-11 Thread Fabian Vogt
fvogt edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D9830 To: fvogt, #frameworks, dfaure

D9830: Fix KFilePreviewGenerator::LayoutBlocker

2018-01-11 Thread Fabian Vogt
fvogt created this revision. fvogt added reviewers: Frameworks, dfaure. Restricted Application added a project: Frameworks. fvogt requested review of this revision. REVISION SUMMARY QAbstractItemViews does layout in a timer event handler, to avoid unnecessary layout calculations. Changes

D9830: Fix KFilePreviewGenerator::LayoutBlocker

2018-01-12 Thread Fabian Vogt
This revision was automatically updated to reflect the committed changes. Closed by commit R241:248941a0f4b4: Fix KFilePreviewGenerator::LayoutBlocker (authored by fvogt). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9830?vs=25186=25201 REVISION DETAIL

D9830: Fix KFilePreviewGenerator::LayoutBlocker

2018-01-12 Thread Fabian Vogt
fvogt added a comment. In https://phabricator.kde.org/D9830#189769, @dfaure wrote: > Nice investigation and fix! Do you happen to know the actual change in Qt which broke this? Wondering if it should 1) be mentioned in the commit log, 2) lead to an #if QT_VERSION. > But if it's just

D9830: Fix KFilePreviewGenerator::LayoutBlocker

2018-01-12 Thread Fabian Vogt
fvogt added a comment. In https://phabricator.kde.org/D9830#189776, @dfaure wrote: > I certainly hope QListView doesn't trigger a nested event loop, that would be horribly nasty and the cause for a million more problems (up to and including crashes when closing the view/window while

D9966: [KIO] Fix issues with sharing of file descriptor

2018-02-03 Thread Fabian Vogt
fvogt added a comment. In https://phabricator.kde.org/D9966#199916, @dfaure wrote: > The early push was so we don't release 5.43 (planned for today) without this security fix at all, in case of no answer... Timing is not that important. As long as the other issues mentioned in

D9966: [KIO] Fix issues with sharing of file descriptor

2018-01-27 Thread Fabian Vogt
fvogt requested changes to this revision. fvogt added a comment. This revision now requires changes to proceed. Almost done! I just noticed some small issues with reading errno after calling `execWithElevatedPrivileges(...)` (I only left a single comment, the other places are basically

D9966: [KIO] Fix issues with sharing of file descriptor

2018-01-27 Thread Fabian Vogt
fvogt resigned from this revision. fvogt added a comment. Works for me. I'll leave the final review to someone else (@dfaure?). REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D9966 To: chinmoyr, #frameworks, thiago Cc: ngraham, fvogt, lbeltrame, dfaure, michaelh

D10141: Restore Persistence=session for the file ioslave kauth helper

2018-01-27 Thread Fabian Vogt
fvogt added a comment. There is one issue I have with this. While this is close to the `sudo`-mode of temporary authorization grants, it doesn't work that way as the whole session has full access via file.so. It would be great if this could work with just the application which

Re: Current security issues with KAuth support in KIO

2018-02-05 Thread Fabian Vogt
Hi, Am Sonntag, 4. Februar 2018, 23:47:26 CET schrieb Albert Astals Cid: > So we're having KF5 5.43 next week, has this been figured out? not quite, but it's on the right track (unlikely for 5.43 though). See below. Cheers, Fabian > I find this thread ended too open ended for my taste. > >

D10437: Limit the use of file.so for privilege operation to one application

2018-02-11 Thread Fabian Vogt
fvogt added a comment. If I'm not mistaken, the application sends its own pid to the slave. This means it could just fake it. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10437 To: chinmoyr, #frameworks, dfaure, fvogt Cc: michaelh

D10141: Restore Persistence=session for the file ioslave kauth helper

2018-02-09 Thread Fabian Vogt
fvogt added a comment. In https://phabricator.kde.org/D10141#203726, @aacid wrote: > In https://phabricator.kde.org/D10141#203664, @fvogt wrote: > > > In https://phabricator.kde.org/D10141#203545, @chinmoyr wrote: > > > > > In https://phabricator.kde.org/D10141#197039, @fvogt

D10141: Restore Persistence=session for the file ioslave kauth helper

2018-02-09 Thread Fabian Vogt
fvogt added a comment. In https://phabricator.kde.org/D10141#203545, @chinmoyr wrote: > In https://phabricator.kde.org/D10141#197039, @fvogt wrote: > > > There is one issue I have with this. While this is close to the `sudo`-mode of temporary authorization grants, it doesn't work

D10437: Limit the use of file.so for privilege operation to one application

2018-02-11 Thread Fabian Vogt
fvogt added a comment. In D10437#204377 , @chinmoyr wrote: > The whole work is being done inside KIO::Job. If the application uses regular Jobs then I can't see how it can fake it. By not using KIO or using a modified KIO. Never assume

D10437: Limit the use of file.so for privilege operation to one application

2018-02-11 Thread Fabian Vogt
fvogt added a comment. In D10437#204413 , @chinmoyr wrote: > In D10437#204382 , @fvogt wrote: > > > In D10437#204377 , @chinmoyr wrote: > > > > > The

D9593: Fix crash in setMainWindow on wayland

2018-01-03 Thread Fabian Vogt
fvogt added a comment. Ping? This should probably go into 5.42. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D9593 To: fvogt, graesslin, dfaure Cc: #frameworks

D9593: Fix crash in setMainWindow on wayland

2018-01-04 Thread Fabian Vogt
This revision was automatically updated to reflect the committed changes. Closed by commit R236:9a46dd5199ee: Fix crash in setMainWindow on wayland (authored by fvogt). REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9593?vs=24554=24706 REVISION DETAIL

D13535: Do not cancel old clipboard selection if it is same as the new one.

2018-06-21 Thread Fabian Vogt
This revision was automatically updated to reflect the committed changes. Closed by commit R127:0e580ae9bdc5: Do not cancel old clipboard selection if it is same as the new one. (authored by michalsrb, committed by fvogt). REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE

D7423: [WIP/assistance needed] Populate UDS_CREATION_TIME on Linux if statx system call is available

2018-08-17 Thread Fabian Vogt
fvogt requested changes to this revision. fvogt added a comment. This revision now requires changes to proceed. On neon it won't work as the kernel everything is built against (so the minimum API/ABI) is too old. You'll either have to hack around that by messing with include paths or use

D14723: Add option to disable KWallet entirely in the new wallet dialog

2018-08-11 Thread Fabian Vogt
fvogt added a comment. In D14723#306659 , @aacid wrote: > Isn't this actually bad for the privacy goal? AFAIK some apps will just write the password in cleartext in a config file when kwallet is not present. It's not selected by default

  1   2   3   4   >