From: "alx (Alejandro Colomar)" Date: 2022-07-05T08:52:09+00:00 Subject: [ruby-core:109140] [Ruby master Bug#18893] Don't redefine memcpy(3) Issue #18893 has been updated by alx (Alejandro Colomar). nobu (Nobuyoshi Nakada) wrote in #note-5: > Do we need to force the function to be inlined? You can, but it's not the best solution. I suiggest the following patches (if you like them, I'll send them to the mailing list): ```diff commit f9da464637f6f705d334fa5dc0da7a32f4624173 (HEAD -> master) Author: Alejandro Colomar Date: Tue Jul 5 10:50:13 2022 +0200 Simplify memcpy() wrapper Although ISO C specifies that memcpy(dest, NULL, 0) is Undefined Behavior (hence the [[gnu::nonnull]] attribute in glibc), all known libc implementations produce a no-op for that call, so we don't need to add a useless branch for that case. However, removing the branch changes the behavior for memcpy(dest, NULL, n). Before, we were producing a no-op. Now, it will produce UB (very likely, a program crash, by dereferencing NULL). For such a programmer error, the best thing to do is to crash hard, so that the programmer realizes the problem. Signed-off-by: Alejandro Colomar diff --git a/include/ruby/internal/memory.h b/include/ruby/internal/memory.h index f044e61ef9..b03ec5dbfd 100644 --- a/include/ruby/internal/memory.h +++ b/include/ruby/internal/memory.h @@ -655,12 +655,7 @@ RBIMPL_ATTR_RETURNS_NONNULL() inline void * ruby_nonempty_memcpy(void *dest, const void *src, size_t n) { - if (n) { - return memcpy(dest, src, n); - } - else { - return dest; - } + return memcpy(dest, src, n); } RBIMPL_SYMBOL_EXPORT_END() #undef memcpy commit b5a09c246703908fc39c4835d2206eb7a080a4bc Author: Alejandro Colomar Date: Tue Jul 5 10:42:28 2022 +0200 ruby_nonempty_memcpy(): Use C99 inline 'static' should only be ever used in source (.c) files, since its meaning is that "this function will be private to this translation unit", which forces the compiler to create a different standalone private function for every translation unit, in cases where a function is not inlined by the compiler. This has triggered some warnings in code that includes our header, where an inline (C99 inline) function called (unknowingly) our memcpy() wrapper, which is ruby_nonempty_memcpy() (after the preprocessor). An inline function shouldn't call a static function (since the compiler expects that 'static' will only be used in source files, not header files), and therefore the warning. This commit removes that warning, and also improves this function, by not emitting duplicate code. Signed-off-by: Alejandro Colomar diff --git a/include/ruby/internal/memory.h b/include/ruby/internal/memory.h index aa3464465d..f044e61ef9 100644 --- a/include/ruby/internal/memory.h +++ b/include/ruby/internal/memory.h @@ -652,7 +652,7 @@ RBIMPL_ATTR_RETURNS_NONNULL() /* At least since 2004, glibc's annotates memcpy to be * __attribute__((__nonnull__(1, 2))). However it is safe to pass NULL to the * source pointer, if n is 0. Let's wrap memcpy. */ -static inline void * +inline void * ruby_nonempty_memcpy(void *dest, const void *src, size_t n) { if (n) { diff --git a/memory.c b/memory.c new file mode 100644 index 0000000000..bfa080c835 --- /dev/null +++ b/memory.c @@ -0,0 +1,12 @@ +/********************************************************************** + + memory.c - Memory + + Copyright (C) 2022 Alejandro Colomar + +**********************************************************************/ + +#include "internal/memory.h" + + +extern inline void *ruby_nonempty_memcpy(void *dest, const void *src, size_t n); ``` ---------------------------------------- Bug #18893: Don't redefine memcpy(3) https://bugs.ruby-lang.org/issues/18893#change-98281 * Author: alx (Alejandro Colomar) * Status: Open * Priority: Normal * Assignee: shyouhei (Shyouhei Urabe) * ruby -v: ruby 3.0.4p208 (2022-04-12 revision 3fa771dded) [x86_64-linux-gnu] * Backport: 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN ---------------------------------------- It is Undefined Behavior, by any standard ever issued. See what I have in my system right now: ``` alx@asus5775:/usr/include$ grepc memcpy ./string.h:43: extern void *memcpy (void *__restrict __dest, const void *__restrict __src, size_t __n) __THROW __nonnull ((1, 2)); ./x86_64-linux-gnu/ruby-3.0.0/rb_mjit_min_header-3.0.4.h:1520: extern void *memcpy (void *__restrict __dest, const void *__restrict __src, size_t __n) __attribute__ ((__nothrow__ , __leaf__)) __attribute__ ((__nonnull__ (1, 2))); ./x86_64-linux-gnu/ruby-3.0.0/rb_mjit_min_header-3.0.4.h:1670: extern __inline __attribute__ ((__always_inline__)) __attribute__ ((__gnu_inline__)) __attribute__ ((__artificial__)) void * __attribute__ ((__nothrow__ , __leaf__)) memcpy (void *__restrict __dest, const void *__restrict __src, size_t __len) { return __builtin___memcpy_chk (__dest, __src, __len, __builtin_object_size (__dest, 0)); } ./ruby-3.0.0/ruby/internal/memory.h:278: #define memcpy ruby_nonempty_memcpy ./x86_64-linux-gnu/ruby-3.0.0/rb_mjit_min_header-3.0.4.h:22679: #define memcpy ruby_nonempty_memcpy $ grepc ruby_nonempty_memcpy ./ruby-3.0.0/ruby/internal/memory.h:266: static inline void * ruby_nonempty_memcpy(void *dest, const void *src, size_t n) { if (n) { return memcpy(dest, src, n); } else { return dest; } } ./x86_64-linux-gnu/ruby-3.0.0/rb_mjit_min_header-3.0.4.h:5673: __attribute__((__nonnull__ (1))) __attribute__((__returns_nonnull__)) static inline void * ruby_nonempty_memcpy(void *dest, const void *src, size_t n) { if (n) { return memcpy(dest, src, n); } else { return dest; } } ``` Some code that I maintain includes some ruby headers, which end up defining memcpy(3) to that thing. Then, my code calls memcpy(3) from a function, which happens to be inline (yes, inline, not static inline, which is a horrible thing), and I get an error from the compiler, for using a static function within a non-static inline function. So, I'd like you to please remove that definition from public headers, and refrain from redefining any ISO C functions. If not, please define it to something not broken (and yes, static inline is broken, as it produces duplicated code when not inlined; you could use [[gnu::always_inline]] if you want to keep static, but don't). Just C99 inline would be reasonable. See also: Thanks, Alex -- https://bugs.ruby-lang.org/ Unsubscribe: