Crypt method blowfish corrupts large files in 7.4. Okay in 7.3.

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Create a large file (e.g. 2 MB file).  You can use the num.txt.gz attachment 
but gunzip it first.

2. Use vim to load num.txt and configure these settings.
set cm=blowfish
set key=num
w num2.txt

3. Use vim to load num2.txt.  Enter "num" for the password.  Go to the end of 
the file.  The file is corrupted.

What is the expected output? What do you see instead?
It should show the original file contents.

What version of the product are you using? On what operating system?
VIM - Vi IMproved 7.4 (2013 Aug 10, compiled Aug 10 2013 14:33:40)

This was on a Windows 8.1 machine, but it happens on Mac and Linux as well.
Vim 7.3.46 does not have this problem.  Using crypt method zip does not have 
this problem.

Original issue reported on code.google.com by stlee...@gmail.com on 28 May 2015 at 3:59

Attachments:

• num.txt.gz

[GoogleCodeExporter]

It sounds like you're using the installer from vim.org, which is severely 
outdated.  Testing with the latest code in Mercurial, I don't see your problem.

I'd suggest using the installers the Cream project makes available 
(http://sourceforge.net/projects/cream/files/Vim), since it seems this has 
already been fixed.

Original comment by vega.james@gmail.com on 28 May 2015 at 4:21

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Actually we've seen problems encrypting large files at least as late as   
7.4.608: https://groups.google.com/d/topic/vim_use/NFvXEopHBOI/discussion 

I don't think those problems were ever tracked down and fixed. 

Perhaps this problem is similar. However, I cannot reproduce it with the   
attached test file, using either blowfish2 or blowfish methods in 7.4.682.   
If you're still able to reproduce, try disabling swap files before   
encrypting. That's what worked as a workaround in the linked thread.

Original comment by benjamin...@rockwellcollins.com on 28 May 2015 at 5:43

[GoogleCodeExporter]

I can reproduce it with a plain 7.4 Windows built. I don't know the crypt code 
very well, but I'll see if I can find the problem.

Original comment by chrisbr...@googlemail.com on 28 May 2015 at 7:23

[GoogleCodeExporter]

I downloaded and installed gvim-7-4-711.exe and while the problem doesn't 
happen exactly as I state now, the corruption is still there.  Just page down 
to the second or third page and there is "garbage" in the middle of the file 
instead of the end of the file.

One other thing I noticed now is that even with the zip encryption, corruption 
occurred in the middle of the file.

Vim 7.3 does not have this problem so it seems like a regression.  Also using 
blowfish takes much longer to encrypt and decrypt compared to Vim 7.3.

Original comment by stlee...@gmail.com on 28 May 2015 at 7:35

[GoogleCodeExporter]

Did you try disabling swap files?

Original comment by fritzoph...@gmail.com on 28 May 2015 at 10:04

[GoogleCodeExporter]

Using valgrind, I can see problems using Vim-7.4.729 on Linux x86_64.

When doing:

$ valgrind --log-file=vg.log --leak-check=yes --track-fds=yes --num-callers=50 
--track-origins=yes vim num.txt

:set cm=blowfish
:set key=num

After this, I can already see bugs with valgrind:

===
==7298== Memcheck, a memory error detector
==7298== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==7298== Using Valgrind-3.10.0.SVN and LibVEX; rerun with -h for copyright info
==7298== Command: ./vim num.txt
==7298== Parent PID: 2437
==7298== 
==7298== Use of uninitialised value of size 8
==7298==    at 0x410614: bf_e_block (blowfish.c:360)
==7298==    by 0x410E46: bf_e_cblock (blowfish.c:396)
==7298==    by 0x41157E: crypt_blowfish_encode (blowfish.c:618)
==7298==    by 0x415291: crypt_encode (crypt.c:448)
==7298==    by 0x4C2A24: ml_encrypt_data (memline.c:4874)
==7298==    by 0x5E0D57: mf_write_block (memfile.c:1139)
==7298==    by 0x5E0C56: mf_write (memfile.c:1095)
==7298==    by 0x5E06FD: mf_release (memfile.c:870)
==7298==    by 0x5DFFD8: mf_get (memfile.c:453)
==7298==    by 0x4BB1AB: ml_upd_block0 (memline.c:940)
==7298==    by 0x4BA837: ml_set_crypt_key (memline.c:515)
==7298==    by 0x512B91: did_set_string_option (option.c:6166)
==7298==    by 0x510853: do_set (option.c:4894)
==7298==    by 0x47C23A: ex_set (ex_docmd.c:11996)
==7298==    by 0x46D148: do_one_cmd (ex_docmd.c:2940)
==7298==    by 0x469EC8: do_cmdline (ex_docmd.c:1133)
==7298==    by 0x4F89F7: nv_colon (normal.c:5393)
==7298==    by 0x4F1DD8: normal_cmd (normal.c:1160)
==7298==    by 0x5D7A65: main_loop (main.c:1347)
==7298==    by 0x5D738A: main (main.c:1047)
==7298==  Uninitialised value was created by a heap allocation
==7298==    at 0x4C2AB80: malloc (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7298==    by 0x4E0F46: lalloc (misc2.c:926)
==7298==    by 0x4E0E1E: alloc (misc2.c:821)
==7298==    by 0x5DF70A: mf_open (memfile.c:135)
==7298==    by 0x4BA2D6: ml_open (memline.c:316)
==7298==    by 0x4057AE: open_buffer (buffer.c:98)
==7298==    by 0x5D96C7: create_windows (main.c:2692)
==7298==    by 0x5D703D: main (main.c:881)
==7298== 

.... skip many other access to uninitialized memory ....

==7298== Syscall param write(buf) points to uninitialised byte(s)
==7298==    at 0x6646870: __write_nocancel (syscall-template.S:81)
==7298==    by 0x49AA51: write_eintr (fileio.c:10393)
==7298==    by 0x5E0D81: mf_write_block (memfile.c:1145)
==7298==    by 0x5E0C56: mf_write (memfile.c:1095)
==7298==    by 0x5E06FD: mf_release (memfile.c:870)
==7298==    by 0x5DFFD8: mf_get (memfile.c:453)
==7298==    by 0x4BB1AB: ml_upd_block0 (memline.c:940)
==7298==    by 0x4BA837: ml_set_crypt_key (memline.c:515)
==7298==    by 0x512B91: did_set_string_option (option.c:6166)
==7298==    by 0x510853: do_set (option.c:4894)
==7298==    by 0x47C23A: ex_set (ex_docmd.c:11996)
==7298==    by 0x46D148: do_one_cmd (ex_docmd.c:2940)
==7298==    by 0x469EC8: do_cmdline (ex_docmd.c:1133)
==7298==    by 0x4F89F7: nv_colon (normal.c:5393)
==7298==    by 0x4F1DD8: normal_cmd (normal.c:1160)
==7298==    by 0x5D7A65: main_loop (main.c:1347)
==7298==    by 0x5D738A: main (main.c:1047)
==7298==  Address 0x8afb575 is 373 bytes inside a block of size 4,096 alloc'd
==7298==    at 0x4C2AB80: malloc (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7298==    by 0x4E0F46: lalloc (misc2.c:926)
==7298==    by 0x4E0E1E: alloc (misc2.c:821)
==7298==    by 0x4C2960: ml_encrypt_data (memline.c:4862)
==7298==    by 0x5E0D57: mf_write_block (memfile.c:1139)
==7298==    by 0x5E0C56: mf_write (memfile.c:1095)
==7298==    by 0x5E06FD: mf_release (memfile.c:870)
==7298==    by 0x5DFFD8: mf_get (memfile.c:453)
==7298==    by 0x4BB1AB: ml_upd_block0 (memline.c:940)
==7298==    by 0x4BA837: ml_set_crypt_key (memline.c:515)
==7298==    by 0x512B91: did_set_string_option (option.c:6166)
==7298==    by 0x510853: do_set (option.c:4894)
==7298==    by 0x47C23A: ex_set (ex_docmd.c:11996)
==7298==    by 0x46D148: do_one_cmd (ex_docmd.c:2940)
==7298==    by 0x469EC8: do_cmdline (ex_docmd.c:1133)
==7298==    by 0x4F89F7: nv_colon (normal.c:5393)
==7298==    by 0x4F1DD8: normal_cmd (normal.c:1160)
==7298==    by 0x5D7A65: main_loop (main.c:1347)
==7298==    by 0x5D738A: main (main.c:1047)
==7298==  Uninitialised value was created by a heap allocation
==7298==    at 0x4C2AB80: malloc (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7298==    by 0x4E0F46: lalloc (misc2.c:926)
==7298==    by 0x4E0E1E: alloc (misc2.c:821)
==7298==    by 0x5DF70A: mf_open (memfile.c:135)
==7298==    by 0x4BA2D6: ml_open (memline.c:316)
==7298==    by 0x4057AE: open_buffer (buffer.c:98)
==7298==    by 0x5D96C7: create_windows (main.c:2692)
==7298==    by 0x5D703D: main (main.c:881)
==7298== 

Then when doing :w num2.txt  I see this additional error:

==7298== Invalid read of size 1
==7298==    at 0x492722: buf_write (fileio.c:4524)
==7298==    by 0x4593AC: do_write (ex_cmds.c:2766)
==7298==    by 0x458E9B: ex_write (ex_cmds.c:2575)
==7298==    by 0x46D148: do_one_cmd (ex_docmd.c:2940)
==7298==    by 0x469EC8: do_cmdline (ex_docmd.c:1133)
==7298==    by 0x4F89F7: nv_colon (normal.c:5393)
==7298==    by 0x4F1DD8: normal_cmd (normal.c:1160)
==7298==    by 0x5D7A65: main_loop (main.c:1347)
==7298==    by 0x5D738A: main (main.c:1047)
==7298==  Address 0x9e724e0 is 0 bytes after a block of size 4,096 alloc'd
==7298==    at 0x4C2AB80: malloc (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7298==    by 0x4E0F46: lalloc (misc2.c:926)
==7298==    by 0x4E0E1E: alloc (misc2.c:821)
==7298==    by 0x5E090A: mf_alloc_bhdr (memfile.c:952)
==7298==    by 0x5DFE44: mf_new (memfile.c:392)
==7298==    by 0x4C0817: ml_new_data (memline.c:3545)
==7298==    by 0x4BEE36: ml_append_int (memline.c:2793)
==7298==    by 0x4BE78B: ml_append (memline.c:2567)
==7298==    by 0x48EE9B: readfile (fileio.c:2252)
==7298==    by 0x405930: open_buffer (buffer.c:147)
==7298==    by 0x5D96C7: create_windows (main.c:2692)
==7298==    by 0x5D703D: main (main.c:881)
==7298== 

I don't see any error with valgrind when adding the -u NONE option:

$ vim -u NONE num.txt

:set cm=blowfish
:set key=num
:w num2.txt
:q!

-> No error found with valgrind.

So something in my ~/.vimrc is triggering a bug.
I have not had the time to investigate further yet, but I will try later.
I will at least narrow down what triggers the valgrind error in my ~/.vimrc.

Original comment by dominiqu...@gmail.com on 29 May 2015 at 4:37

[GoogleCodeExporter]

I can reproduce the valgrind errors reported in my previous
comment using the following minimalistic 2-lines ~/.vimrc:

$ cat ~/.vimrc
set maxmem=256
set cm=blowfish

Then typing...

$ valgrind --log-file=vg.log --track-origins=yes ./vim --noplugin num.txt -c 
'set key=num'

... causes the valgrind errors reported in my previous comment in vg.log. 
num.txt is the file attached in this ticket by bug submitter.

The bug does not happen if num.txt is truncated to a few lines.
So it only happens with large files.

I'm using:
* vim-7.4.729 (huge)
* on Linux x86_64 (xubuntu-14.04.2)

Original comment by dominiqu...@gmail.com on 30 May 2015 at 5:42

[GoogleCodeExporter]

I think, I found the problem. We are setting the key and the cryptmethod. When
writing the buffer, Vim gets each line and when getting a line, it will need
to find the block, it belongs to, which means, it will eventually call mf_get()
to read a new block from the disc. But, in mf_get() we are calling
ml_decrypt_data() to decrypt the block, if a key is set. Unfortunately, the
block isn't actually encrypted yet...

That means, it happens only, if the file does need several blocks and more than
'maxmem' kbytes.

My guess is, this happens, because on Windows 'mm' defaults to 2048, while on
Linux, maxmem defaults to half of the memory available, so therefore, this bug
does not trigger there, because it can read all blocks into memory when reading
the buffer and does not need to get the block from the disc again.

Unfortunately, I don't have a patch available yet.

As side note, it looks strange, that the maxmem limit on Windows is so much
smaller than on Linux. One might consider increasing this limit on Windows as
well.

Original comment by chrisbr...@googlemail.com on 3 Jun 2015 at 9:54

[GoogleCodeExporter]

I think, this patch fixes it.

Original comment by chrisbr...@googlemail.com on 4 Jun 2015 at 8:55

• Added labels: patch

Attachments:

• issue_369.diff

[GoogleCodeExporter]

Looks like the patch does not encrypt any data blocks, because it checks the 
flag before doing encryption, and the flag is never set at that point.

Original comment by brammool...@gmail.com on 9 Jun 2015 at 9:43

[GoogleCodeExporter]

Well, as i said, i don't know much about the encryption, but I checked, that it 
created an encrypted file when running Vim with that patch. At least I got a 
binary file that I was able to decrypt on Linux as well (where the maxmem 
option does not trigger).

Also the flag is set in ml_encrypt_data() aroud line 4881 (looking from the 
diff).

Original comment by chrisbr...@googlemail.com on 9 Jun 2015 at 2:43

[GoogleCodeExporter]

Fixed by patch 7.4.730.

Original comment by brammool...@gmail.com on 9 Jun 2015 at 4:37

• Changed state: Fixed