Bug 397545 - Assertion on write error (e.g. disk full situation) for KCompressionDevice
Summary: Assertion on write error (e.g. disk full situation) for KCompressionDevice
Status: RESOLVED FIXED
Alias: None
Product: frameworks-karchive
Classification: Frameworks and Libraries
Component: general (show other bugs)
Version: unspecified
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: David Faure
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-08-17 10:37 UTC by Christoph Cullmann
Modified: 2018-08-18 14:31 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Christoph Cullmann 2018-08-17 10:37:36 UTC
See the phabricator entry:

https://phabricator.kde.org/D14890#310356

Backtrace is:

ASSERT: "d->avail_out > 0" in file /home/cullmann/kde/src/frameworks/karchive/src/knonefilter.cpp, line 128

Thread 1 "kwrite" received signal SIGABRT, Aborted.
0x00007ffff156708b in raise () from /lib64/libc.so.6
(gdb) bt
#0 0x00007ffff156708b in raise () from /lib64/libc.so.6
#1 0x00007ffff15504e9 in abort () from /lib64/libc.so.6
#2 0x00007ffff22b022b in QMessageLogger::fatal(char const*, ...) const () from /usr/lib64/libQt5Core.so.5
#3 0x00007ffff22af7d9 in qt_assert(char const*, char const*, int) () from /usr/lib64/libQt5Core.so.5
#4 0x00007ffff04b44fb in KNoneFilter::copyData (this=0x7fffdc0278f0) at /home/cullmann/kde/src/frameworks/karchive/src/knonefilter.cpp:128
#5 0x00007ffff04b44c6 in KNoneFilter::compress (this=0x7fffdc0278f0, finish=true) at /home/cullmann/kde/src/frameworks/karchive/src/knonefilter.cpp:123
#6 0x00007ffff04b277e in KCompressionDevice::writeData (this=0x7fffffffc220, data=0x0, len=0)

at /home/cullmann/kde/src/frameworks/karchive/src/kcompressiondevice.cpp:343
#7 0x00007ffff23ddddd in QIODevice::write(char const*, long long) () from /usr/lib64/libQt5Core.so.5
#8 0x00007ffff04b1f1d in KCompressionDevice::close (this=0x7fffffffc220) at /home/cullmann/kde/src/frameworks/karchive/src/kcompressiondevice.cpp:165
#9 0x00007ffff77ee7fd in Kate::TextBuffer::save (this=0x72a930, filename=...)

at /home/cullmann/kde/src/frameworks/ktexteditor/src/buffer/katetextbuffer.cpp:858


On disk full situations (or I assume any other situation with write errors), the error handling is a bit suboptimal with the assert.
Comment 1 David Faure 2018-08-17 23:10:12 UTC
Here when I do `kate /tmp/mnt/test.gz` and I write a ton of text and then save, I don't get any assert, but also no error and a zero bytes file on disk!

kcompressiondevice.cpp:173 calls QFile::close (which is where the writing actually happens) but doesn't check for error...
(gdb) p d->filter->device()->errorString()
$21 = "No space left on device"

All this checking of write calls... when in fact there's buffering everywhere nowadays and it all happens on close, which doesn't even return a bool :-)

KArchive fix: https://phabricator.kde.org/D14913
ktexteditor fix on top of D14890: http://www.davidfaure.fr/2018/check_save.diff
Comment 2 Christoph Cullmann 2018-08-18 05:41:45 UTC
Did you try it with my patch from https://phabricator.kde.org/D14890 applied?
For me it only happens with the None filter variant of the KDevFilter, which is worked around in that.
Could you try with some non-gz file but without the shortcut in the line 

QScopedPointer<QIODevice> saveFile((type == KCompressionDevice::None) ? static_cast<QIODevice*>(new QFile(filename)) : static_cast<QIODevice*>(new KCompressionDevice(filename, type)));

aka always using the KCompressionDevice?
Comment 3 David Faure 2018-08-18 11:02:17 UTC
Git commit b0b2ee5a149356515beff46b3b0591ea71121bc0 by David Faure.
Committed on 18/08/2018 at 11:02.
Pushed by dfaure into branch 'master'.

KCompressionDevice: propagate errors from QIODevice::close()

Summary:
QFile::close() doesn't return a value, but sets an error string instead,
we need to check for that and set it in KCompressionDevice so apps can check for that in turn.

Test Plan:
Saving into /tmp/mnt/test.gz with kate, with the setup described in
https://phabricator.kde.org/D14890

Reviewers: cullmann

Reviewed By: cullmann

Subscribers: kde-frameworks-devel

Tags: #frameworks

Differential Revision: https://phabricator.kde.org/D14913

M  +35   -1    autotests/kcompressiondevicetest.cpp
M  +3    -0    autotests/kcompressiondevicetest.h
M  +36   -5    src/kcompressiondevice.cpp
M  +11   -1    src/kcompressiondevice.h

https://commits.kde.org/karchive/b0b2ee5a149356515beff46b3b0591ea71121bc0
Comment 4 David Faure 2018-08-18 11:37:39 UTC
Ah, I failed to see the comment about the avail_out assert only happening with NoneFilter. Reopening, since that's still to be fixed.
Comment 5 David Faure 2018-08-18 11:44:27 UTC
Still can't reproduce the assert though.

In ktexteditor/src/buffer/katetextbuffer.cpp I do
-    QScopedPointer<QIODevice> saveFile((type == KCompressionDevice::None) ? static_cast<QIODevice*>(new QFile(filename)) : static_cast<QIODevice*>(new KCompressionDevice(filename, type)));
+    QScopedPointer<QIODevice> saveFile(new KCompressionDevice(filename, type));

And then kwrite /tmp/mnt/foobar, paste a huge amount of text, save. No error, 0 bytes file (because I don't have your KCompressionDevice error handling in yet).
Comment 6 David Faure 2018-08-18 11:47:03 UTC
Ah I see, one needs to paste a REALLY huge amount of text ;)

I'll debug it.
Comment 7 Christoph Cullmann 2018-08-18 11:55:01 UTC
:=)
Yeah, its a really nice corner case issue.
Comment 8 David Faure 2018-08-18 13:41:07 UTC
One solution would be to test stream.status() inside the loop in Kate::TextBuffer::save, since that (now) correctly gets set to WriteFailed. Or at least after the loop, before calling KCompressionDevice::close.

But https://phabricator.kde.org/D14920 fixes that anyhow.
Comment 9 David Faure 2018-08-18 14:03:03 UTC
Git commit f013beaa1a86c2ce06c9d63cf2e5507be1d02e0f by David Faure.
Committed on 18/08/2018 at 14:02.
Pushed by dfaure into branch 'master'.

KCompressionDevice: don't call write after WriteError

Summary:
if an earlier call to write failed, then there is no point in
doing the final write call, it even asserts in the NoneFilter.

Test Plan:
This fixes saving to full disk using a KCompressionDevice set
up with KNoneFilter, in kwrite.

Reviewers: cullmann

Reviewed By: cullmann

Subscribers: kde-frameworks-devel

Tags: #frameworks

Differential Revision: https://phabricator.kde.org/D14920

M  +1    -2    src/kcompressiondevice.cpp

https://commits.kde.org/karchive/f013beaa1a86c2ce06c9d63cf2e5507be1d02e0f
Comment 10 Christoph Cullmann 2018-08-18 14:31:13 UTC
Thanks for the fix.

I removed again my shortcut around the KCompressionDevice for compression == None.

Git commit 50d853c42025fca2128b46642cb7ab041d51d9d8 by Christoph Cullmann.
Committed on 18/08/2018 at 14:24.
Pushed by cullmann into branch 'master'.

simplify error checking, always use the KCompressionDevice now that the
assert/crash is fixed

early out on error in line writing loop

Differential Revision: https://phabricator.kde.org/D14890

M  +17   -20   src/buffer/katetextbuffer.cpp

https://commits.kde.org/ktexteditor/50d853c42025fca2128b46642cb7ab041d51d9d8


Code is simpler again and I added the early out in the loop for line writing, no reason to continue there after first error, good hint, thanks.