Bug #13069: mkmf: ignore linker warnings on OpenBSD

• Status: Closed
• Assignee: -
• Target version: -
• ruby -v: ruby 2.5.0p0 (2017-12-25 revision 61468) [x86_64-linux]
• Backport: 2.3: UNKNOWN, 2.4: DONE, 2.5: DONE

Installing gems with native extensions fails on my OpenBSD machine since
the linker emits warnings causing stderr to not be empty as expected.
The warnings are security recommendations that does not affect the
linked binary. See examples below from my mkmf.log caused by libruby:

warning: warning: strcpy() is almost always misused, please use strlcpy()
warning: warning: strcat() is almost always misused, please use strlcat()
warning: warning: sprintf() is often misused, please use snprintf()
warning: warning: vsprintf() is often misused, please use vsnprintf()

Attached is patch that checks the output for such harmless warnings. Not
sure if this exception should be allowed on all platforms, but instead
wrapped in a RUBY_PLATFORM =~ /openbsd/ conditional or something
similar.

[jeremyevans0 (Jeremy Evans)]

Personally, I think it would be better to just check the return code and ignore the content of the log file. Are there environments that ruby wants to support where compiling returns success when it actually fails? The supplied patch won't handle all of the warnings produced by OpenBSD's linker, though it does handle the most common ones.

[nobu (Nobuyoshi Nakada)]

• Description updated (diff)
• Status changed from Open to Feedback

Seems sprintf nor vsprintf are used on OpenBSD.
Those warnings are always emitted regardless use of the functions?

Nobuyoshi Nakada wrote:

Seems sprintf nor vsprintf are used on OpenBSD.
Those warnings are always emitted regardless use of the functions?

Here's a minimal example causing the errors to be outputted while
dynamically linking against ruby:

$ env "PATH=${DESTDIR}/usr/local/bin:${PATH}" ruby -v
ruby 2.4.0dev (2016-12-24 trunk 57169) [x86_64-openbsd6.0]
$ cat test.c
#include "ruby.h"

int main(void) { return 0; }
$ cc \
    -o test  \
    -I ${DESTDIR}/usr/local/include/ruby-2.4.0/x86_64-openbsd6.0 \
    -I ${DESTDIR}/usr/local/include/ruby-2.4.0 \
    test.c \
    -L ${DESTDIR}/usr/local/lib \
    -lruby \
    -lm \
    -lpthread
usr/local/lib/libruby.so: warning: warning: strcpy() is almost always misused, please use strlcpy()
usr/local/lib/libruby.so: warning: warning: strcat() is almost always misused, please use strlcat()

[jeremyevans0 (Jeremy Evans)]

• Status changed from Feedback to Open

Some of the warnings were addressed in r57189, r57190, and r57191. However, I think we should rethink checking for an empty log file, and rely purely on the return code:

--- lib/mkmf.rb.orig    Sun Dec 25 09:49:06 2016
+++ lib/mkmf.rb Sun Dec 25 09:49:51 2016
@@ -388,7 +388,7 @@ module MakeMakefile
         result = nil
         Logging.postpone do |log|
           output = IO.popen(libpath_env, command, &:read)
-          result = ($?.success? and File.zero?(log.path))
+          result = $?.success?
           output
         end
         result

The only reason I can think of to not rely purely on the return code is if we want to support environments where the command would return 0 in the case where it failed. Are there such environments?

The approach of the current code is similar to defaulting to -Werror when compiling C, which is almost always a bad idea when distributing C code unless you know every possible environment you want to support (which ruby does not).

[jeremyevans0 (Jeremy Evans)]

I think this is a better fix. This keeps the :werror behavior in Logging::postpone, it just turns off the use of :werror by default. Users that want werror behavior by default can set $werror = true.

--- lib/mkmf.rb.orig
+++ lib/mkmf.rb
@@ -88,6 +88,7 @@ module MakeMakefile
   $static = nil
   $config_h = '$(arch_hdrdir)/ruby/config.h'
   $default_static = $static
+  $werror = false unless defined?($werror)

   unless defined? $configure_args
     $configure_args = {}
@@ -611,7 +612,7 @@ def with_cppflags(flags)
   end

   def try_cppflags(flags, opts = {})
-    try_header(MAIN_DOES_NOTHING, flags, {:werror => true}.update(opts))
+    try_header(MAIN_DOES_NOTHING, flags, {:werror => $werror}.update(opts))
   end

   def append_cppflags(flags, *opts)
@@ -633,7 +634,7 @@ def with_cflags(flags)
   end

   def try_cflags(flags, opts = {})
-    try_compile(MAIN_DOES_NOTHING, flags, {:werror => true}.update(opts))
+    try_compile(MAIN_DOES_NOTHING, flags, {:werror => $werror}.update(opts))
   end

   def append_cflags(flags, *opts)
@@ -655,7 +656,7 @@ def with_ldflags(flags)
   end

   def try_ldflags(flags, opts = {})
-    try_link(MAIN_DOES_NOTHING, flags, {:werror => true}.update(opts))
+    try_link(MAIN_DOES_NOTHING, flags, {:werror => $werror}.update(opts))
   end

   def append_ldflags(flags, *opts)
@@ -1408,7 +1409,7 @@ def convertible_int(type, headers = nil, opts = nil, &b)
           u = "unsigned " if signed > 0
           prelude << "extern rbcv_typedef_ foo();"
           compat = UNIVERSAL_INTS.find {|t|
-            try_compile([prelude, "extern #{u}#{t} foo();"].join("\n"), opts, :werror=>true, &b)
+            try_compile([prelude, "extern #{u}#{t} foo();"].join("\n"), opts, :werror=>$werror, &b)
           }
         end
         if compat

This basically reverts the changes made in r50215 and r50216, except that it allows enabling werror globally via $werror. The commit messages for both commits do not describe why the code was changed to default to {:werror=>true}. From looking at the history of mkmf, the werror code was originally added in r30107 to work around declaration conflict warnings, so maybe we want to keep :werror=>true in the final diff block. r35101 states that the exit code may be lost on mingw, so we may want to default $werror to true on mingw if that is still the case.

I think we need to change the current default of treating warnings as errors. We should handle broken environments that do not use return codes correctly on an exception basis.

Thansk for addressing the warnings. Your proposed patch solves my problem.

[akihikodaki (Akihiko Odaki)]

• File mkmf.patch mkmf.patch added
• ruby -v set to ruby 2.5.0p0 (2017-12-25 revision 61468) [x86_64-linux]

The issue is affecting my gem, cld3-ruby.
Build failure on OpenBSD · Issue #16 · akihikodaki/cld3-ruby

Those warnings are always emitted regardless use of the functions?

I reviewed the source code of Binutils and GCC and found no means to suppress the warning.

I understand many link warnings result in runtime failure, but it is just insane to make all warnings errors. Warnings are not errors because they may not result in actual failure.

The impact of this issue is severe. It would disable many native extensions on OpenBSD. On the other hand, though those warnings sound ridiculous, OpenBSD should not be blamed because they are just adding warnings, not errors. mkmf should be fixed.

I have two suggestions to address this kind of issue:

1. Redirect linker's stderr to mkmf's stderr, and optionally ask the user if he thinks it is fine to continue.
2. Execute the output instead of catching stderr to detect runtime failures if not cross compiling.

I attached a patch to implement suggestion 2.

[nobu (Nobuyoshi Nakada)]

• Status changed from Open to Closed

Applied in changeset trunk|r62007.

---

mkmf.rb: ignore linker warnings

• lib/mkmf.rb (try_ldflags): ignore linker warnings. they cause
  unexpected failures on OpenBSD. [ruby-core:78827] [Bug #13069]

[nobu (Nobuyoshi Nakada)]

Could you let us know where those functions are used?
It's better to get rid of them, I think.

[hsbt (Hiroshi SHIBATA)]

• Backport changed from 2.1: UNKNOWN, 2.2: UNKNOWN, 2.3: UNKNOWN to 2.3: UNKNOWN, 2.4: UNKNOWN, 2.5: REQUIRED

[nagachika (Tomoyuki Chikanaga)]

• Backport changed from 2.3: UNKNOWN, 2.4: UNKNOWN, 2.5: REQUIRED to 2.3: UNKNOWN, 2.4: UNKNOWN, 2.5: DONE

ruby_2_5 r64979 merged revision(s) 62007,62024.

[usa (Usaku NAKAMURA)]

• Backport changed from 2.3: UNKNOWN, 2.4: UNKNOWN, 2.5: DONE to 2.3: UNKNOWN, 2.4: DONE, 2.5: DONE

ruby_2_4 r65117 merged revision(s) 62007,62024.