Package: leafpad
Version: 0.8.18.1-5
X-Debbugs-CC: bladeboy200...@gmail.com
Severity: critical
Some mishandling of error return code in the saving function of
leafpad can lead to data loss.
When a file ($FILE) is saved in leafpad, it calls the functions
file_save_real(file.c:180):
- The content on the GtkTextBuffer is transformed in a gchar*,
- Then the file $FILE is opened (file.c:220) in write mode (which
truncate $FILE content)
- The gchar* is written to $FILE (file.c:226) but not necessary yet flushed
(on my computer there is an autoflush only if more than 4k of data),
- Leafpad calls the function gtk_text_buffer_set_modified()
(file.c:232) for the UI layer
- Then the $FILE is closed (file.c:233) which causes the unflushed
content to be finally written to disk.
- Sadly the result of that fclose() (file.c:233) is not checked at all.
---
1) first problem, the "truncating side-effect"
The call to gtk_text_buffer_set_modified() (file.c:232) triggers a
"modified-changed" signal.
There is a callback (view.c:354) that links that "modified-changed"
signal to cb_modified_changed.
-> the function cb_modified_changed(view.c:218) calls (view.c:224)
the function get_file_basename()
-> the function get_file_basename(file.c:42) calls (file.c:66) the
function check_file_writable()
-> the function check_file_writable(file.c:31) :
- calls (file.c:35) the function fopen() on the same $FILE in
APPEND mode to check if it's writable
- and (file.c:36) that second handle on $FILE is closed
Because in the file_save_real(file.c:180), gtk_text_buffer_set_modified()
is called before the fclose() somehow 2 handles on $FILE exists at the
same time,
the second handle being closed before the first.
On a local filesystem (ext4) it usually behave somehow without errors:
when the file is fclosed any unflushed content is then written on disk.
Here is the strace when trying to save 2 bytes in /tmp/file.txt (an
ext filesystem)
file.c:220:file_save_real() | open("/tmp/file.txt",
O_WRONLY|O_CREAT|O_TRUNC, 0666) = 13
file.c:035:check_file_writable() | open("/tmp/file.txt",
O_WRONLY|O_CREAT|O_APPEND, 0666) = 14
file.c:036:check_file_writable() | close(14) = 0
file.c:233:file_save_real() | *write(13, "X!", 2) = 2* 🗸
file.c:233:file_save_real() | close(13) = 0
Until here it seems fine,
but that can have a dramatic side-effect when used with gvfs over
sftp, a bug happens!
( see https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=680418
and https://bugs.launchpad.net/ubuntu/+source/leafpad/+bug/708829 )
gfvs+sftp don't like having concurrently 2 file pointers on the same file,
and trying to write data on the oldest, after having closed the
youngest ... fails and the file end up truncated.
After having mounted the sftp://localhost/tmp/file.txt via gvfs, the same strace
file.c:220:file_save_real() |
open("/run/user/1000/gvfs/sftp:host=localhost,user=me/tmp/file.txt",
O_WRONLY|O_CREAT|O_TRUNC, 0666) = 13
file.c:035:check_file_writable() |
open("/run/user/1000/gvfs/sftp:host=localhost,user=me/tmp/file.txt",
O_WRONLY|O_CREAT|O_APPEND, 0666) = 14
file.c:036:check_file_writable() | close(14) = 0
file.c:233:file_save_real() | *write(13, "X!", 2) = -1
EOPNOTSUPP (Operation not supported) 🗴 *😱
file.c:233:file_save_real() | close(13) = 0
Solution proposed:
As previously proposed in 2012 (and by Max Rockatanski
<bladeboy200...@gmail.com> in 2016) on bug=680418,
the call to fclose() should be done before the call to
gtk_text_buffer_set_modified().
The data should also probably be fflushed and fsynced just after the
fwrite.
---
2) second problem, the silent (No space left on device)
The result of fclose() (file.c:233) being not checked at all,
if there is not enough space on disk to write data when flushing, the
remaining data is currently silently lost.
For example try creating a ram drive of 4kb:
mkdir -p /tmp/tmpfs4k && mount -o size=4k -t tmpfs none /tmp/tmpfs4k
Then try to write 4097 bytes into /tmp/tmpfs4k/test (with a strace) :
file.c:220:file_save_real() | open("/tmp/tmpfs4k/test",
O_WRONLY|O_CREAT|O_TRUNC, 0666) = 13
file.c:226:file_save_real() | write(13,
"XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"..., 4096) = 4096
file.c:035:check_file_writable() | open("/tmp/tmpfs4k/test",
O_WRONLY|O_CREAT|O_APPEND, 0666) = 14
file.c:036:check_file_writable() | close(14) = 0
file.c:233:file_save_real() | *write(13, "!", 1) = -1
ENOSPC (No space left on device) 🗴 😱*
file.c:233:file_save_real() | close(13) = 0
Solution proposed:
The return code for fopen should be checked and a feedback should be
provided to the user if an error occur.
(and the same thing should be done if we add a fflush and a fsync)
