Project

General

Profile

Bug #13069

mkmf: ignore linker warnings on OpenBSD

Added by anton (Anton Lindqvist) over 2 years ago. Updated 6 months ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Target version:
-
ruby -v:
ruby 2.5.0p0 (2017-12-25 revision 61468) [x86_64-linux]
[ruby-core:78827]

Description

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.


Files

ruby-mkmf_openbsd.diff (880 Bytes) ruby-mkmf_openbsd.diff anton (Anton Lindqvist), 12/25/2016 09:05 AM
mkmf.patch (4.19 KB) mkmf.patch Test link result by executing the output akihikodaki (Akihiko Odaki), 01/22/2018 10:19 AM

Associated revisions

Revision 4f03a239
Added by nobu (Nobuyoshi Nakada) about 1 year ago

mkmf.rb: ignore linker warnings

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

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@62007 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 62007
Added by nobu (Nobuyoshi Nakada) about 1 year ago

mkmf.rb: ignore linker warnings

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

Revision 62007
Added by nobu (Nobuyoshi Nakada) about 1 year ago

mkmf.rb: ignore linker warnings

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

Revision 7184e03e
Added by nobu (Nobuyoshi Nakada) about 1 year ago

mkmf.rb: werror on mswin

  • lib/mkmf.rb (MakeMakefile#try_ldflags): enable warning checking on mswin, link.exe warns -l options but does not fail. [Bug #13069]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@62024 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 62024
Added by nobu (Nobuyoshi Nakada) about 1 year ago

mkmf.rb: werror on mswin

  • lib/mkmf.rb (MakeMakefile#try_ldflags): enable warning checking on mswin, link.exe warns -l options but does not fail. [Bug #13069]

Revision 62024
Added by nobu (Nobuyoshi Nakada) about 1 year ago

mkmf.rb: werror on mswin

  • lib/mkmf.rb (MakeMakefile#try_ldflags): enable warning checking on mswin, link.exe warns -l options but does not fail. [Bug #13069]

Revision b014387f
Added by nagachika (Tomoyuki Chikanaga) 6 months ago

merge revision(s) 62007,62024: [Backport #13069]

    mkmf.rb: ignore linker warnings

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

    mkmf.rb: werror on mswin

    * lib/mkmf.rb (MakeMakefile#try_ldflags): enable warning checking
      on mswin, link.exe warns -l options but does not fail.
      [Bug #13069]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/branches/ruby_2_5@64979 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 64979
Added by nagachika (Tomoyuki Chikanaga) 6 months ago

merge revision(s) 62007,62024: [Backport #13069]

mkmf.rb: ignore linker warnings

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

mkmf.rb: werror on mswin

* lib/mkmf.rb (MakeMakefile#try_ldflags): enable warning checking
  on mswin, link.exe warns -l options but does not fail.
  [Bug #13069]

Revision 2a13a71e
Added by usa (Usaku NAKAMURA) 6 months ago

merge revision(s) 62007,62024: [Backport #13069]

    mkmf.rb: ignore linker warnings

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

    mkmf.rb: werror on mswin

    * lib/mkmf.rb (MakeMakefile#try_ldflags): enable warning checking
      on mswin, link.exe warns -l options but does not fail.
      [Bug #13069]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/branches/ruby_2_4@65117 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 65117
Added by usa (Usaku NAKAMURA) 6 months ago

merge revision(s) 62007,62024: [Backport #13069]

mkmf.rb: ignore linker warnings

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

mkmf.rb: werror on mswin

* lib/mkmf.rb (MakeMakefile#try_ldflags): enable warning checking
  on mswin, link.exe warns -l options but does not fail.
  [Bug #13069]

History

Updated by jeremyevans0 (Jeremy Evans) over 2 years ago

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.

Updated by nobu (Nobuyoshi Nakada) over 2 years ago

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

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

Updated by anton (Anton Lindqvist) over 2 years ago

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()

Updated by jeremyevans0 (Jeremy Evans) over 2 years ago

  • 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).

Updated by jeremyevans0 (Jeremy Evans) over 2 years ago

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.

Updated by anton (Anton Lindqvist) over 2 years ago

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

Updated by akihikodaki (Akihiko Odaki) about 1 year ago

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

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.

#8

Updated by nobu (Nobuyoshi Nakada) about 1 year ago

  • 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]

Updated by nobu (Nobuyoshi Nakada) about 1 year ago

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

#10

Updated by hsbt (Hiroshi SHIBATA) 8 months ago

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

Updated by nagachika (Tomoyuki Chikanaga) 6 months ago

  • 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.

Updated by usa (Usaku NAKAMURA) 6 months ago

  • 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.

Also available in: Atom PDF