[patch] fixed crash in blowfish.c on linux86_64

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[patch] fixed crash in blowfish.c on linux86_64

Dominique Pellé
Hi

I just ran "make test" with Vim-7.3a BETA (2177:3cb515c62e9c)
and it causes a segfault in test71 on Linux x86_64.
The same test runs without problem on Linux x86 so bug is specific to 64-bit.

(gdb) bt
#0  0x00002b2612abb7e7 in kill () from /lib64/libc.so.6
#1  0x000000000051e56b in may_core_dump () at os_unix.c:3107
#2  0x000000000051e500 in mch_exit (r=1) at os_unix.c:3072
#3  0x00000000004b1374 in getout (exitval=1) at main.c:1373
#4  0x00000000004dfb23 in preserve_exit () at misc1.c:8406
#5  0x000000000051c80f in deathtrap (sigarg=11) at os_unix.c:1076
#6  <signal handler called>
#7  0x0000000000418bfd in bf_e_block (p_xl=0x7fffdf0f77a0,
p_xr=0x7fffdf0f7798) at blowfish.c:336
#8  0x0000000000419400 in bf_key_init (password=0x829460 "password")
at blowfish.c:435
#9  0x0000000000419555 in bf_self_test () at blowfish.c:500
#10 0x00000000004197d6 in blowfish_self_test () at blowfish.c:573
#11 0x00000000005160c8 in set_num_option (opt_idx=54, varp=0x898948
"\001", value=1, errbuf=0x7fffdf0f78d0 "\360\205\017\337\377\177", err
buflen=80, opt_flags=0) at option.c:7890
#12 0x000000000050fe0c in do_set (arg=0x8624d0 "1", opt_flags=0) at
option.c:4410
#13 0x0000000000479517 in ex_set (eap=0x7fffdf0f7a90) at ex_docmd.c:11034
#14 0x000000000046b4b8 in do_one_cmd (cmdlinep=0x7fffdf0f8108,
sourcing=0, cstack=0x7fffdf0f7c60, fgetline=0x480414 <getexline>,
cookie=0x
0) at ex_docmd.c:2629
#15 0x0000000000468aae in do_cmdline (cmdline=0x0, getline=0x480414
<getexline>, cookie=0x0, flags=0) at ex_docmd.c:1098
#16 0x00000000004fa85a in nv_colon (cap=0x7fffdf0f8220) at normal.c:5226
#17 0x00000000004f37a0 in normal_cmd (oap=0x7fffdf0f82f0, toplevel=1)
at normal.c:1188
#18 0x00000000004b1098 in main_loop (cmdwin=0, noexmode=0) at main.c:1212
#19 0x00000000004b0b7c in main (argc=9, argv=0x7fffdf0f85f8) at main.c:956

329        static void
330    bf_e_block(p_xl, p_xr)
331        long_u *p_xl;
332        long_u *p_xr;
333    {
334        long_u temp, xl = *p_xl, xr = *p_xr;
335
336--->    F1(0) F2(1) F1(2) F2(3) F1(4) F2(5) F1(6) F2(7)
337        F1(8) F2(9) F1(10) F2(11) F1(12) F2(13) F1(14) F2(15)
338        xl ^= pax[16]; xr ^= pax[17];
339        temp = xl; xl = xr; xr = temp;
340        *p_xl = xl; *p_xr = xr;
341    }

If I put all the F*(?) statements on different lines, I see
that it crashes in F1(6).

314 #define F1(i) \
315     xl ^= pax[i]; \
316     xr ^= ((sbx[0][xl>>24] + \
317     sbx[1][(xl&0xFF0000)>>16]) ^ \
318     sbx[2][(xl&0xFF00)>>8]) + \
319     sbx[3][xl&0xFF];

(gdb) p xl
$2 = 1053699622217
(gdb) p xl >> 24
$3 = 62805
(gdb) p sbx[0][xl>>24]
Cannot access memory at address 0x8c1008

sbx is declared as: "static long_u sbx[4][256];"
so access to sbx[0][62805] causes an invalid memory access.

I see that:
* sizeof(long_u) is 4 on Linux x86
* sizeof(long_u) is 8 on Linux x86_64

The different long_u type is causing the different behavior.

long_u is defined in vim.h:391 as:

 388 # if !defined(_MSC_VER) || (_MSC_VER < 1300)
 389 #  define __w64
 390 # endif
 391 typedef unsigned long __w64     long_u;

On Linux, __64 is empty, so long_u is just:

typedef unsigned long  long_u;

... which is 32 bits or 64 bits depending on systems.

We could use int64_t  (defined in stdint.h) but that's
only defined for c99.

Or we could use "unsigned long long", but that's also
only defined in c99 I think.

Is there a portable way of defining a unsigned 64-bit number
that will work with old compilers?  I don't think so.

Anyway, looking at code in blowfish.c, it seems to me that
computations were meant to be done in unsigned 32 bits
and not in 64 bits. It gives different results since intermediate
computations exceed 64 bits.

Attached patch fixes it (at least all tests pass) but please review it.

-- Dominique

--
You received this message from the "vim_dev" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php

fixed-crash-blowfish.c.patch (882 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [patch] fixed crash in blowfish.c on linux86_64

Mosh-3
Dominique,

I am able to reproduce the crash on 64bit linux (gcc
x86_64-linux-gnu), the fix is:

1  blowfish blocks should always be 32bits: i.e. unsigned int, not
unsigned long.
2. My BIG_ENDIAN macro usage was incompatible with linux/stdlib, this
was caught by the self test.
3 The same self test as windows should work on all machines.

Attaching the unified diffs from blowfish.[ch] vs patch4/blowfish.[ch]

thanks for the details,
mohsin


2010/5/16 Dominique Pellé <[hidden email]>:

> Hi
>
> I just ran "make test" with Vim-7.3a BETA (2177:3cb515c62e9c)
> and it causes a segfault in test71 on Linux x86_64.
> The same test runs without problem on Linux x86 so bug is specific to 64-bit.
>
> (gdb) bt
> #0  0x00002b2612abb7e7 in kill () from /lib64/libc.so.6
> #1  0x000000000051e56b in may_core_dump () at os_unix.c:3107
> #2  0x000000000051e500 in mch_exit (r=1) at os_unix.c:3072
> #3  0x00000000004b1374 in getout (exitval=1) at main.c:1373
> #4  0x00000000004dfb23 in preserve_exit () at misc1.c:8406
> #5  0x000000000051c80f in deathtrap (sigarg=11) at os_unix.c:1076
> #6  <signal handler called>
> #7  0x0000000000418bfd in bf_e_block (p_xl=0x7fffdf0f77a0,
> p_xr=0x7fffdf0f7798) at blowfish.c:336
> #8  0x0000000000419400 in bf_key_init (password=0x829460 "password")
> at blowfish.c:435
> #9  0x0000000000419555 in bf_self_test () at blowfish.c:500
> #10 0x00000000004197d6 in blowfish_self_test () at blowfish.c:573
> #11 0x00000000005160c8 in set_num_option (opt_idx=54, varp=0x898948
> "\001", value=1, errbuf=0x7fffdf0f78d0 "\360\205\017\337\377\177", err
> buflen=80, opt_flags=0) at option.c:7890
> #12 0x000000000050fe0c in do_set (arg=0x8624d0 "1", opt_flags=0) at
> option.c:4410
> #13 0x0000000000479517 in ex_set (eap=0x7fffdf0f7a90) at ex_docmd.c:11034
> #14 0x000000000046b4b8 in do_one_cmd (cmdlinep=0x7fffdf0f8108,
> sourcing=0, cstack=0x7fffdf0f7c60, fgetline=0x480414 <getexline>,
> cookie=0x
> 0) at ex_docmd.c:2629
> #15 0x0000000000468aae in do_cmdline (cmdline=0x0, getline=0x480414
> <getexline>, cookie=0x0, flags=0) at ex_docmd.c:1098
> #16 0x00000000004fa85a in nv_colon (cap=0x7fffdf0f8220) at normal.c:5226
> #17 0x00000000004f37a0 in normal_cmd (oap=0x7fffdf0f82f0, toplevel=1)
> at normal.c:1188
> #18 0x00000000004b1098 in main_loop (cmdwin=0, noexmode=0) at main.c:1212
> #19 0x00000000004b0b7c in main (argc=9, argv=0x7fffdf0f85f8) at main.c:956
>
> 329        static void
> 330    bf_e_block(p_xl, p_xr)
> 331        long_u *p_xl;
> 332        long_u *p_xr;
> 333    {
> 334        long_u temp, xl = *p_xl, xr = *p_xr;
> 335
> 336--->    F1(0) F2(1) F1(2) F2(3) F1(4) F2(5) F1(6) F2(7)
> 337        F1(8) F2(9) F1(10) F2(11) F1(12) F2(13) F1(14) F2(15)
> 338        xl ^= pax[16]; xr ^= pax[17];
> 339        temp = xl; xl = xr; xr = temp;
> 340        *p_xl = xl; *p_xr = xr;
> 341    }
>
> If I put all the F*(?) statements on different lines, I see
> that it crashes in F1(6).
>
> 314 #define F1(i) \
> 315     xl ^= pax[i]; \
> 316     xr ^= ((sbx[0][xl>>24] + \
> 317     sbx[1][(xl&0xFF0000)>>16]) ^ \
> 318     sbx[2][(xl&0xFF00)>>8]) + \
> 319     sbx[3][xl&0xFF];
>
> (gdb) p xl
> $2 = 1053699622217
> (gdb) p xl >> 24
> $3 = 62805
> (gdb) p sbx[0][xl>>24]
> Cannot access memory at address 0x8c1008
>
> sbx is declared as: "static long_u sbx[4][256];"
> so access to sbx[0][62805] causes an invalid memory access.
>
> I see that:
> * sizeof(long_u) is 4 on Linux x86
> * sizeof(long_u) is 8 on Linux x86_64
>
> The different long_u type is causing the different behavior.
>
> long_u is defined in vim.h:391 as:
>
>  388 # if !defined(_MSC_VER) || (_MSC_VER < 1300)
>  389 #  define __w64
>  390 # endif
>  391 typedef unsigned long __w64     long_u;
>
> On Linux, __64 is empty, so long_u is just:
>
> typedef unsigned long  long_u;
>
> ... which is 32 bits or 64 bits depending on systems.
>
> We could use int64_t  (defined in stdint.h) but that's
> only defined for c99.
>
> Or we could use "unsigned long long", but that's also
> only defined in c99 I think.
>
> Is there a portable way of defining a unsigned 64-bit number
> that will work with old compilers?  I don't think so.
>
> Anyway, looking at code in blowfish.c, it seems to me that
> computations were meant to be done in unsigned 32 bits
> and not in 64 bits. It gives different results since intermediate
> computations exceed 64 bits.
>
> Attached patch fixes it (at least all tests pass) but please review it.
>
> -- Dominique
>
--
You received this message from the "vim_dev" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php
Reply | Threaded
Open this post in threaded view
|

Re: [patch] fixed crash in blowfish.c on linux86_64

Dominique Pellé
Mosh wrote:

> Dominique,
>
> I am able to reproduce the crash on 64bit linux (gcc
> x86_64-linux-gnu), the fix is:
>
> 1  blowfish blocks should always be 32bits: i.e. unsigned int, not
> unsigned long.
> 2. My BIG_ENDIAN macro usage was incompatible with linux/stdlib, this
> was caught by the self test.
> 3 The same self test as windows should work on all machines.
>
> Attaching the unified diffs from blowfish.[ch] vs patch4/blowfish.[ch]
>
> thanks for the details,
> mohsin

1/  "unsigned int" is not guaranteed to be 32 bits.
It is typically 32-bits, but it may also be 16 or 64 bits.

2/  To test whether it's big or small endian, I think that the
configure script is more adapted to that.  I don't think
that using '4321' is portable.  See what gcc manual
says about multichar:

http://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html

-Wno-multichar
    Do not warn if a multicharacter constant (`'FOOF'') is used.
    Usually they indicate a typo in the user's code, as they
    have implementation-defined values, and should not be
    used in portable code.

Regards
-- Dominique

--
You received this message from the "vim_dev" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php
Reply | Threaded
Open this post in threaded view
|

Re: [patch] fixed crash in blowfish.c on linux86_64

James Vega-3
In reply to this post by Mosh-3
On Sun, May 16, 2010 at 08:14:08PM -0700, Mosh wrote:
> Dominique,
>
> I am able to reproduce the crash on 64bit linux (gcc
> x86_64-linux-gnu), the fix is:
>
> 1  blowfish blocks should always be 32bits: i.e. unsigned int, not
> unsigned long.

Autoconf has the AC_TYPE_UINT32_T[0] macro which can be used to ensure
there's a uint32_t defined.

> 2. My BIG_ENDIAN macro usage was incompatible with linux/stdlib, this
> was caught by the self test.

There's also the AC_C_BIGENDIAN[1] macro that may be useful for
detecting little-endian vs. big-endian.

[0]: http://www.gnu.org/software/autoconf/manual/html_node/Particular-Types.html#index-AC_005fTYPE_005fUINT32_005fT-791
[1]: http://www.gnu.org/software/autoconf/manual/html_node/C-Compiler.html#index-AC_005fC_005fBIGENDIAN-873
--
James
GPG Key: 1024D/61326D40 2003-09-02 James Vega <[hidden email]>

signature.asc (205 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [patch] fixed crash in blowfish.c on linux86_64

Bram Moolenaar

James Vega wrote:

> On Sun, May 16, 2010 at 08:14:08PM -0700, Mosh wrote:
> > Dominique,
> >
> > I am able to reproduce the crash on 64bit linux (gcc
> > x86_64-linux-gnu), the fix is:
> >
> > 1  blowfish blocks should always be 32bits: i.e. unsigned int, not
> > unsigned long.
>
> Autoconf has the AC_TYPE_UINT32_T[0] macro which can be used to ensure
> there's a uint32_t defined.
>
> > 2. My BIG_ENDIAN macro usage was incompatible with linux/stdlib, this
> > was caught by the self test.
>
> There's also the AC_C_BIGENDIAN[1] macro that may be useful for
> detecting little-endian vs. big-endian.
>
> [0]: http://www.gnu.org/software/autoconf/manual/html_node/Particular-Types.html#index-AC_005fTYPE_005fUINT32_005fT-791
> [1]: http://www.gnu.org/software/autoconf/manual/html_node/C-Compiler.html#index-AC_005fC_005fBIGENDIAN-873

Using configure to figure out the types sounds like the right solution
for Unix.

I prefer to stay as close to the original implementation as possible.
Any small mistake can significantly lower the quality of the encryption.

We also need to have #ifdefs of compilation arguments in the Makefile
for other systems than Unix.  Perhaps we can simply assume that uint32_t
exists and wait for someone to complain when it doesn't work.  And add a
check in the blowfish test function.

--
hundred-and-one symptoms of being an internet addict:
54. You start tilting your head sideways to smile. :-)

 /// Bram Moolenaar -- [hidden email] -- http://www.Moolenaar.net   \\\
///        sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\        download, build and distribute -- http://www.A-A-P.org        ///
 \\\            help me help AIDS victims -- http://ICCF-Holland.org    ///

--
You received this message from the "vim_dev" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php
Reply | Threaded
Open this post in threaded view
|

Re: [patch] fixed crash in blowfish.c on linux86_64

Bram Moolenaar
In reply to this post by Mosh-3

Moshin wrote:

> I am able to reproduce the crash on 64bit linux (gcc
> x86_64-linux-gnu), the fix is:
>
> 1  blowfish blocks should always be 32bits: i.e. unsigned int, not
> unsigned long.
> 2. My BIG_ENDIAN macro usage was incompatible with linux/stdlib, this
> was caught by the self test.
> 3 The same self test as windows should work on all machines.
>
> Attaching the unified diffs from blowfish.[ch] vs patch4/blowfish.[ch]

I have merged the suggested changes and will push the new version as
soon as the repository is open again.

Please check again for any remaining problems.

--
Be thankful to be in a traffic jam, because it means you own a car.

 /// Bram Moolenaar -- [hidden email] -- http://www.Moolenaar.net   \\\
///        sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\        download, build and distribute -- http://www.A-A-P.org        ///
 \\\            help me help AIDS victims -- http://ICCF-Holland.org    ///

--
You received this message from the "vim_dev" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php
Reply | Threaded
Open this post in threaded view
|

Re: [patch] fixed crash in blowfish.c on linux86_64

Mosh-3
2010/5/17 Bram Moolenaar <[hidden email]>:

>
> Moshin wrote:
>
>> I am able to reproduce the crash on 64bit linux (gcc
>> x86_64-linux-gnu), the fix is:
>>
>> 1  blowfish blocks should always be 32bits: i.e. unsigned int, not
>> unsigned long.
>> 2. My BIG_ENDIAN macro usage was incompatible with linux/stdlib, this
>> was caught by the self test.
>> 3 The same self test as windows should work on all machines.
>>
>> Attaching the unified diffs from blowfish.[ch] vs patch4/blowfish.[ch]
>
> I have merged the suggested changes and will push the new version as
> soon as the repository is open again.
>
> Please check again for any remaining problems.
Bram,

Noticed one problem in my macro usage in blowfish.c,

wrong
< #if defined(BF_BIG_ENDIAN) && (BF_BIG_ENDIAN == 0)
---
fix, should have been
> #if defined(BF_BIG_ENDIAN) && (BF_BIG_ENDIAN == 1)

If you have a cleaner ENDIAN macros, you can replace this.

--
Changing all the types to uint32_t seems cleaner, I should
have used it earlier:

 typedef union {
-    unsigned int ul[2];
-    unsigned char uc[8];
+    uint32_t ul[2];
+    uint8_t  uc[8];
 } block8;

Attaching the files again, tested on windows and linux x86-64,
tested on big endian solaris long back (but not recently).

thanks,
Mohsin.

>
> --
> Be thankful to be in a traffic jam, because it means you own a car.
>
>  /// Bram Moolenaar -- [hidden email] -- http://www.Moolenaar.net   \\\
> ///        sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
> \\\        download, build and distribute -- http://www.A-A-P.org        ///
>  \\\            help me help AIDS victims -- http://ICCF-Holland.org    ///
>

--
You received this message from the "vim_dev" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php

patch5.zip (22K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [patch] fixed crash in blowfish.c on linux86_64

Bram Moolenaar

Moshin wrote:

> >> I am able to reproduce the crash on 64bit linux (gcc
> >> x86_64-linux-gnu), the fix is:
> >>
> >> 1  blowfish blocks should always be 32bits: i.e. unsigned int, not
> >> unsigned long.
> >> 2. My BIG_ENDIAN macro usage was incompatible with linux/stdlib, this
> >> was caught by the self test.
> >> 3 The same self test as windows should work on all machines.
> >>
> >> Attaching the unified diffs from blowfish.[ch] vs patch4/blowfish.[ch]
> >
> > I have merged the suggested changes and will push the new version as
> > soon as the repository is open again.
> >
> > Please check again for any remaining problems.
>
> Bram,
>
> Noticed one problem in my macro usage in blowfish.c,
>
> wrong
> < #if defined(BF_BIG_ENDIAN) && (BF_BIG_ENDIAN == 0)
> ---
> fix, should have been
> > #if defined(BF_BIG_ENDIAN) && (BF_BIG_ENDIAN == 1)
>
> If you have a cleaner ENDIAN macros, you can replace this.

I have used the autoconf macro that James Vega suggested.
I assume all MS-Windows compilers use little endian (are there any power
PC users remaining?).

I added an explicit check in the self test for the little/big endian to
be picked wrong.

> Changing all the types to uint32_t seems cleaner, I should
> have used it earlier:
>
>  typedef union {
> -    unsigned int ul[2];
> -    unsigned char uc[8];
> +    uint32_t ul[2];
> +    uint8_t  uc[8];
>  } block8;

Already did that.  I'm using char_u for the 8 bit type, as this is all
over the Vim source code.

Any other places where these types need to be used?

> Attaching the files again, tested on windows and linux x86-64,
> tested on big endian solaris long back (but not recently).

Please compare to what is now in the Mercurial repository.

--
hundred-and-one symptoms of being an internet addict:
61. Your best friends know your e-mail address, but neither your phone number
    nor the address where you live.

 /// Bram Moolenaar -- [hidden email] -- http://www.Moolenaar.net   \\\
///        sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\        download, build and distribute -- http://www.A-A-P.org        ///
 \\\            help me help AIDS victims -- http://ICCF-Holland.org    ///

--
You received this message from the "vim_dev" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php