Bug #6303: dln_load and rb_w32_check_imported cause segfault in Ruby 1.9.3 for some extension

[Luis Lavena]

Hello,

NOTE: Reporting this here since bugs.ruby-lang.org seems to be down.

Recently a user reported to RubyInstaller project issues when loading
a Ruby 1.9.2 compiled extension under Ruby 1.9.3:

https://groups.google.com/d/msg/rubyinstaller/aSezE2LwfQs/TDZvPG3X5mUJ

Which I was able to study a bit better:
https://groups.google.com/d/msg/rubyinstaller/aSezE2LwfQs/UGKlButpNfMJ

To add more, my last comment was:

"Is worth to mention that this do not fail against 1.9.2 (either
building or running) but dln_load mechanism on Ruby 1.9.2 differs from
Ruby 1.9.3 and highly unlikely is going to change."

I'm not convinced by my last comment and I do believe this is a bug.
db2cli.dll links to MSVCR80 and even so, it loads properly under
1.9.2.

Looking closely to what rb_w32_check_imported does, it is supposed to
verify that the extension being loaded it is indeed using the right
ruby dll.

But is failing to obtain Name from pii (PIMAGE_IMPORT_BY_NAME struct)

I can't find any reference to dbghelp (which provides
ImageDirectoryEntryToData) being included or linked in
msvcrt-libruby191.dll

For sure I'm missing something, specially why is failing to obtain
this extension information when works for others.

Thank you.
--
Luis Lavena
AREA 17
-
Perfection in design is achieved not when there is nothing more to add,
but rather when there is nothing more to take away.
Antoine de Saint-Exupéry

[Heesob Park]

The segfault is due to the invalid pointer reference in getting PIMAGE_IMPORT_BY_NAME pointer like this:

PIMAGE_IMPORT_BY_NAME pii = (PIMAGE_IMPORT_BY_NAME)((char *)ext + (size_t)pint->u1.AddressOfData);

Consider the following imports dump list of ibm_db.so

DB2CLI.dll
          63317274 Import Address Table
          63317078 Import Name Table
                 0 time date stamp
                 0 Index of first forwarder reference

                  Ordinal  1300
                  Ordinal  1301
               6C SQLDriverConnectW@32
               52 SQLConnectW@28
                  Ordinal     9
                  Ordinal  1303
                  Ordinal    58
               64 SQLDescribeColW@36
                  Ordinal     4
               4A SQLColumnPrivilegesW@36
               4E SQLColumnsW@36
               FB SQLPrimaryKeysW@28
               98 SQLForeignKeysW@52
               FF SQLProcedureColumnsW@36
              103 SQLProceduresW@28
              133 SQLSpecialColumnsW@40
              137 SQLStatisticsW@36
              13B SQLTablePrivilegesW@28
              13F SQLTablesW@36
               7A SQLExecDirectW@12
               F7 SQLPrepareW@12

...

As you can see, the name table entry has two types: ordinal type and name type.
But, the rb_w32_check_imported function overlooked ordinal case.

Refer to http://nienie.com/~masapico/api_ImageDirectoryEntryToData.html

Here is a patch:

diff --git a/dln.c b/dln.c.new
index e3dff9b..58042e1 100644
--- a/dln.c
+++ b/dln.c.new
@@ -1215,12 +1215,14 @@ rb_w32_check_imported(HMODULE ext, HMODULE mine)
PIMAGE_THUNK_DATA pint = (PIMAGE_THUNK_DATA)((char *)ext + desc->Characteristics);
PIMAGE_THUNK_DATA piat = (PIMAGE_THUNK_DATA)((char *)ext + desc->FirstThunk);
while (piat->u1.Function) {
- PIMAGE_IMPORT_BY_NAME pii = (PIMAGE_IMPORT_BY_NAME)((char *)ext + (size_t)pint->u1.AddressOfData);
- static const char prefix[] = "rb_";
- const char *name = (const char *)pii->Name;
- if (strncmp(name, prefix, sizeof(prefix) - 1) == 0) {
- FARPROC addr = GetProcAddress(mine, name);
- if (addr) return (FARPROC)piat->u1.Function == addr;
+ if(!IMAGE_SNAP_BY_ORDINAL(pint->u1.Ordinal)) {
+ PIMAGE_IMPORT_BY_NAME pii = (PIMAGE_IMPORT_BY_NAME)((char *)ext + (size_t)pint->u1.AddressOfData);
+ static const char prefix[] = "rb_";
+ const char *name = (const char *)pii->Name;
+ if (strncmp(name, prefix, sizeof(prefix) - 1) == 0) {
+ FARPROC addr = GetProcAddress(mine, name);
+ if (addr) return (FARPROC)piat->u1.Function == addr;
+ }
}
piat++;
pint++;

[Nobuyoshi Nakada]

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

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

---

• dln.c (rb_w32_check_imported): skip ordinal entries. patched by phasis68 (Heesob Park) at . [Bug #6303]

[Heesob Park]

The changeset r35352 is not same to my patch and the bug #6303 is not fixed.

After applying r35352,
require 'mswin32/ibm_db'
falls into infinite loop and runs forever.

The line
if (IMAGE_SNAP_BY_ORDINAL(pint->u1.Ordinal)) continue;

should be

if (IMAGE_SNAP_BY_ORDINAL(pint->u1.Ordinal)) {pint++;piat++;continue;}

[Luis Lavena]

• ruby -v set to 1.9.3
• Target version set to 1.9.3
• % Done changed from 100 to 80
• Category set to core
• Status changed from Closed to Assigned
• Assignee set to Nobuyoshi Nakada

[Nobuyoshi Nakada]

• % Done changed from 80 to 100
• Status changed from Assigned to Closed

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

---

• dln.c (rb_w32_check_imported): skip ordinal entries. based on a patch by phasis68 (Heesob Park) at . [Bug #6303]