[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 <[e-mail]>
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 <[e-mail]>

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 <[e-mail]>
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 <[e-mail]>

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 <string.h> 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 <[e-mail]>
+
+**********************************************************************/
+
+#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: <https://www.greenend.org.uk/rjk/tech/inline.html>

Thanks,

Alex



-- 
https://bugs.ruby-lang.org/

Unsubscribe: <mailto:[e-mail]?subject=unsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>