blackwinter / ruby-filemagic

Ruby bindings to the magic(4) library, revised.
https://blackwinter.github.iom/ruby-filemagic
146 stars 34 forks source link

Fix segfault on buffer(nil) when compiled with gcc #24

Closed ypresto closed 7 years ago

ypresto commented 7 years ago

I faced segfault on Docker ruby:2.3.3 (Debian Linux) env, but it does not reproduce on OS X. After investigation, I found that there is compiler-level difference between them.

When buffer(nil) is called, this library raises TypeError from StringValuePtr(nil) call at below code, with llvm (OS X).

magic_buffer(ms, StringValuePtr(arg), RSTRING_LEN(arg))

But with GNU gcc, RSTRING_LEN(nil) is evaluated first and causes SEGV.

Evaluation order of args is unspecified by C language specification, so it is expected behavior. http://en.cppreference.com/w/c/language/eval_order

This PR fixes it by calling explicitly calling StringValuePtr() before RSTRING_LEN().

(Below library passed nil to buffer(), so I also PR is sent to fix TypeError itself :) https://github.com/janko-m/shrine/pull/171


Reproduction environment (for args order)

Test code:

#include <stdio.h>

int x = 0;
  int main() {
    printf("%d, %d\n", (x = 3), (x = 4));
    printf("%d\n", x);
    return 0;
}

OS X (llvm)

$ gcc --version
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 8.1.0 (clang-802.0.42)
Target: x86_64-apple-darwin16.6.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
$ gcc test.c
test.c:5:27: warning: multiple unsequenced modifications to 'x' [-Wunsequenced]
    printf("%d, %d\n", (x = 3), (x = 4));
                          ^        ~
1 warning generated.
$ ./a.out
3, 4
4

First one is evaluated first.

Linux (gcc)

# gcc --version
gcc (Debian 4.9.2-10) 4.9.2
Copyright (C) 2014 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

# gcc test.c
# ./a.out
3, 4
3

Second one is evaluated first...

ypresto commented 7 years ago

fixed..!

blackwinter commented 7 years ago

Thank you for this thorough report and your contribution! I've released version 0.7.2 with the fix.