[ruby-core:109134] [Ruby master Bug#18893] Don't redefine memcpy(3)

Issue #18893 has been updated by alx (Alejandro Colomar).


Actually, ISO C specifies (indirectly) that `memcpy(dest, NULL, 0)` is Undefined Behavior.
See: <https://stackoverflow.com/questions/5243012/is-it-guaranteed-to-be-safe-to-perform-memcpy0-0-0>

ISO C doesn't specify the [[nonnull]] attribute (at least not yet), but that attribute makes sure that you don't invoke UB, so I don't think it's wrong here.

However, I can see reasons for calling `memcpy(dest, NULL, 0)` (e.g., not having to check always before calling memcpy(3)),
and glibc (and all libcs that I know of) implement `memcpy(dest, NULL, 0)` as a no-op (see below for the glibc implementation), so it should be a safe call, as far as libc is concerned,
so maybe ISO C could add a guarantee that it is a safe call.

So, I see reasons to redefine memcpy(3) on your side, to remove the warning.  But I think you could do it in a better way, that doesn't break users:

a - You could use ruby_nonempty_memcpy() directly, and don't redefine memcpy(3).
b - Or you could define ruby_nonempty_memcpy() as a macro, instead of a static inline, so that your memcpy(3) wrapper can be called everywhere.
c - Or you could define ruby_nonempty_memcpy() as a C99 inline function, adding an extern prototype in a .c file, and exporting the function.

I think (b) will be the simplest for everyone.

About using `#undef`, I'm not sure it's legal to do that.  It's likely to work, but I don't think users should have to do this anyway.

Thanks,

Alex

```c
alx@asus5775:~/src/gnu/glibc$ grepc MEMCPY string/memcpy.c 
string/memcpy.c:27:
void *
MEMCPY (void *dstpp, const void *srcpp, size_t len)
{
  unsigned long int dstp = (long int) dstpp;
  unsigned long int srcp = (long int) srcpp;

  /* Copy from the beginning to the end.  */

  /* If there not too few bytes to copy, use word copy.  */
  if (len >= OP_T_THRES)
    {
      /* Copy just a few bytes to make DSTP aligned.  */
      len -= (-dstp) % OPSIZ;
      BYTE_COPY_FWD (dstp, srcp, (-dstp) % OPSIZ);

      /* Copy whole pages from SRCP to DSTP by virtual address manipulation,
	 as much as possible.  */

      PAGE_COPY_FWD_MAYBE (dstp, srcp, len, len);

      /* Copy from SRCP to DSTP taking advantage of the known alignment of
	 DSTP.  Number of bytes remaining is put in the third argument,
	 i.e. in LEN.  This number may vary from machine to machine.  */

      WORD_COPY_FWD (dstp, srcp, len, len);

      /* Fall out and copy the tail.  */
    }

  /* There are just a few bytes to copy.  Use byte memory operations.  */
  BYTE_COPY_FWD (dstp, srcp, len);

  return dstpp;
}


string/memcpy.c:24:
# define MEMCPY memcpy
```
```c
alx@asus5775:~/src/gnu/glibc$ grepc BYTE_COPY_FWD
./sysdeps/generic/memcopy.h:76:
#define BYTE_COPY_FWD(dst_bp, src_bp, nbytes)				      \
  do									      \
    {									      \
      size_t __nbytes = (nbytes);					      \
      while (__nbytes > 0)						      \
	{								      \
	  byte __x = ((byte *) src_bp)[0];				      \
	  src_bp += 1;							      \
	  __nbytes -= 1;						      \
	  ((byte *) dst_bp)[0] = __x;					      \
	  dst_bp += 1;							      \
	}								      \
    } while (0)


./sysdeps/i386/memcopy.h:25:
#define BYTE_COPY_FWD(dst_bp, src_bp, nbytes)				      \
  do {									      \
    int __d0;								      \
    asm volatile(/* Clear the direction flag, so copying goes forward.  */    \
		 "cld\n"						      \
		 /* Copy bytes.  */					      \
		 "rep\n"						      \
		 "movsb" :						      \
		 "=D" (dst_bp), "=S" (src_bp), "=c" (__d0) :		      \
		 "0" (dst_bp), "1" (src_bp), "2" (nbytes) :		      \
		 "memory");						      \
  } while (0)


./sysdeps/powerpc/powerpc32/power4/memcopy.h:61:
#define BYTE_COPY_FWD(dst_bp, src_bp, nbytes)				      \
  do									      \
    {									      \
      size_t __nbytes = (nbytes);					      \
      if (__nbytes & 1)							      \
        {								      \
	  ((byte *) dst_bp)[0] =  ((byte *) src_bp)[0];			      \
	  src_bp += 1;							      \
	  dst_bp += 1;							      \
	  __nbytes -= 1;						      \
        }								      \
      while (__nbytes > 0)						      \
	{								      \
	  byte __x = ((byte *) src_bp)[0];				      \
	  byte __y = ((byte *) src_bp)[1];				      \
	  src_bp += 2;							      \
	  __nbytes -= 2;						      \
	  ((byte *) dst_bp)[0] = __x;					      \
	  ((byte *) dst_bp)[1] = __y;					      \
	  dst_bp += 2;							      \
	}								      \
    } while (0)
```

----------------------------------------
Bug #18893: Don't redefine memcpy(3)
https://bugs.ruby-lang.org/issues/18893#change-98275

* Author: alx (Alejandro Colomar)
* Status: Rejected
* Priority: Normal
* 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>