Bug #8337

Test failure and memory leak with OpenSSL::BN

Added by Hiroshi Shirosaki 12 months ago. Updated 12 months ago.

[ruby-core:54615]
Status:Closed
Priority:Normal
Assignee:Hiroshi Shirosaki
Category:ext
Target version:2.1.0
ruby -v:ruby 2.1.0dev (2013-04-27 trunk 40468) [x86_64-darwin12.3.0] Backport:1.9.3: UNKNOWN, 2.0.0: UNKNOWN

Description

I noticed test failure of testtobn.

http://ci.rubyinstaller.org/job/ruby-trunk-x64-test-all/1137/console
1) Failure:
testtobn(OpenSSL::TestBN) [C:/Users/Worker/Jenkins/workspace/ruby-trunk-x64-build/test/openssl/test_bn.rb:36]:
<#OpenSSL::BN:0x00000017d20188> expected but was
<#OpenSSL::BN:0x00000017d18460>.

sizeof(VALUE) of the following code looks wrong because sizeof(VALUE) > sizeof(long) on x64 Windows.

https://github.com/ruby/ruby/blob/8b29525dadeaba1ba6dc2a9ea5e590aa9d1d825a/ext/openssl/ossl_bn.c#L130

And ALLOC_N() may need xfree().
I've confirmed memory leak with the following script.

$ ruby -rOpenSSL -e 'loop { 1.to_bn }'

Here is a patch to fix above issues.

diff --git a/ext/openssl/osslbn.c b/ext/openssl/osslbn.c
index 4e9734e..3d8e095 100644
--- a/ext/openssl/osslbn.c
+++ b/ext/openssl/ossl
bn.c
@@ -127,15 +127,17 @@ osslbninitialize(int argc, VALUE *argv, VALUE self)
long n = FIX2LONG(str);
unsigned long un = labs(n);

  • for (i = sizeof(VALUE) - 1; 0 <= i; i--) {
  • for (i = sizeof(long) - 1; 0 <= i; i--) {
    bin[i] = un&0xff;
    un >>= 8;
    }

    GetBN(self, bn);
    if (!BN_bin2bn(bin, sizeof(long), bn)) {

  •   xfree(bin);
    ossl_raise(eBNError, NULL);
    

    }

  • xfree(bin);
    if (n < 0) BNsetnegative(bn, 1);
    return self;
    }
    @@ -154,8 +156,10 @@ osslbninitialize(int argc, VALUE *argv, VALUE self)

    GetBN(self, bn);
    if (!BNbin2bn(bin, (int)sizeof(BDIGIT)*RBIGNUMLENINT(str), bn)) {

  •   xfree(bin);
    ossl_raise(eBNError, NULL);
    

    }

  • xfree(bin);
    if (!RBIGNUMSIGN(str)) BNset_negative(bn, 1);
    return self;
    }

Associated revisions

Revision 40513
Added by shirosaki 12 months ago

osslbn.c: fix osslbn_initialize bug with integer

  • ext/openssl/osslbn.c (osslbn_initialize): fix buffer overflow on x64 Windows and memory leak when initializing with integer. [Bug #8337]

History

#1 Updated by Yui NARUSE 12 months ago

Sure, commit please.

#2 Updated by Yui NARUSE 12 months ago

naruse (Yui NARUSE) wrote:

Sure, commit please.

Additionally, ALLOCN for Fixnum can be simply ALLOCAN.

#3 Updated by Anonymous 12 months ago

  • % Done changed from 0 to 100
  • Status changed from Open to Closed

This issue was solved with changeset r40513.
Hiroshi, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


osslbn.c: fix osslbn_initialize bug with integer

  • ext/openssl/osslbn.c (osslbn_initialize): fix buffer overflow on x64 Windows and memory leak when initializing with integer. [Bug #8337]

#4 Updated by Tomoyuki Chikanaga 12 months ago

  • Status changed from Closed to Assigned
  • Assignee set to Hiroshi Shirosaki

Hello,

I think ALLOCA_N() uses alloca() to allocate memory from machine stack and xfree() is not necessary.

#5 Updated by Hiroshi Shirosaki 12 months ago

  • Status changed from Assigned to Closed

nagachika (Tomoyuki Chikanaga) wrote:

Hello,

I think ALLOCA_N() uses alloca() to allocate memory from machine stack and xfree() is not necessary.

I use ALLOCAN only for Fixnum and ALLOCN is used for Bignum that needs xfree().

https://github.com/ruby/ruby/blob/be4aa330374d42cdead52a94144be189b5054e67/ext/openssl/ossl_bn.c#L145

#6 Updated by Tomoyuki Chikanaga 12 months ago

oh...
I'm sorry for bother you.

Also available in: Atom PDF