Hacker News Reader

Comment by ssp | Hacker News Reader

There is a bug in the memmove() function from ulib.c:

    void*
    memmove(void *vdst, void *vsrc, int n)
    {
      char *dst, *src;
  
      dst = vdst;
      src = vsrc;
      while(n-- > 0)
        *dst++ = *src++;
      return vdst;
    }

If the two blocks overlap, and the address of dst is greater than the address of src, then parts of the source will be overwritten before it can be copied.

View on HN

The original v6 doesn’t have any of the mem* functions, but its bcopy implementation has the same issue. Presumably the convention that bcopy permits overlap had not been established at that time.

I'm not massively great at C but would this be a fix?

    void*
    memmove(void *vdst, void *vsrc, int n)
    {
        char *dst, *src;

        dst = vdst;
        src = vsrc;
        if(*vdst > *vsrc) {
            *vdst += n;
            *vsrc += n;
            while(n-- > 0)
                *dst-- = *src--;
        }
        else if(*vdst < *vsrc)
            while(n-- > 0)
                *dst++ == *src++;
        return vdst;
    }

At least two bugs: the test should compare vdst to vsrc, not * vdst to * vsrc, and in the first branch you're copying vsrc[n] which is out of range (and skipping vsrc[0]).

The first bug I'm confused? Surely we want to use the * because we're comparing the position in memory, not what's in the memory? Or does the * mean "compare what is in that memory slot"?

Second bug, cheers.

http://news.ycombinator.com/item?id=3214807

I see an additional bug in there compared to the original, you're copying the memory below vsrc into the memory below vdst in the first branch. you need to add to dst and src not vdst/vsrc.

Thanks, C rather rusty :)

http://news.ycombinator.com/item?id=3214807

Ok, updated version:

    void*
    memmove(void *vdst, void *vsrc, int n)
    {
        char *dst, *src;

        dst = vdst;
        src = vsrc;
        if(vdst > vsrc) {
            dst = vdst + n;
            src = vsrc + n;
            while(n-- > 0)
                *--dst = *--src;
        }
        else if(vdst < vsrc)
            while(n-- > 0)
                *dst++ == *src++;
        return vdst;
    }

Does this solve the bugs?

There's an == for an =. Also vdst+n is an error, I believe, because sizeof *vdst has no meaning.

reminds me of the intel comment in their implementation of memcpy, something like 'no overlay check as implicit convention'

Yes, the standards say that the behavior of memcpy, as opposed to memmove, is undefined when the arguments overlap.

Right. Quoting from the manpage:

> The memory areas may overlap: copying takes place as though the bytes in src are first copied into a temporary array that does not overlap src or dest, and the bytes are then copied from the temporary array to dest.

http://www.kernel.org/doc/man-pages/online/pages/man3/memmov...

Notice how it says 'as though'; I can't see why doing the copy backwards (from end to start) wouldn't work, myself.

Still doesn't work going end to start.

    src starts at 50, ends at 100
    dst starts at 25, ends at 75.

First we copy 100 to 75, then 99 to 74, etc ... now we get to copy from 75 to 50, but we've overwritten it already!

If it's higher, move from the front. If it's lower, start at the back. You have to check each time you do it.

Linux isn't Unix, so are you sure it's the same?

memmove() is in ANSI C standard.

UNIX v6 predates ANSI C by over a decade I think.

Here is a standard implementation I found from some code at work.

    /* memmove.c -- copy memory.                                                                                                                                    
       Copy LENGTH bytes from SOURCE to DEST.  Does not null-terminate.                                                                                             
       In the public domain.                                                                                                                                        
       By David MacKenzie <djm@gnu.ai.mit.edu>.  */                                                                                                                 
                                                                                                                                                                
    #if HAVE_CONFIG_H                                                                                                                                               
    # include <config.h>                                                                                                                                            
    #endif                                                                                                                                                          
                                                                                                                                                                    
    void *                                                                                                                                                          
    memmove (dest, source, length)                                                                                                                                  
         char *dest;                                                                                                                                                
         const char *source;                                                                                                                                        
         unsigned length;                                                                                                                                           
    {                                                                                                                                                               
      char *d0 = dest;                                                                                                                                              
      if (source < dest)                                                                                                                                            
        /* Moving from low mem to hi mem; start at end.  */                                                                                                         
        for (source += length, dest += length; length; --length)                                                                                                    
          *--dest = *--source;                                                                                                                                      
      else if (source != dest)                                                                                                                                      
        {                                                                                                                                                           
          /* Moving from hi mem to low mem; start at beginning.  */                                                                                                 
          for (; length; --length)                                                                                                                                  
            *dest++ = *source++;                                                                                                                                    
        }                                                                                                                                                           
      return (void *) d0;                                                                                                                                           
    }

Reply on news.ycombinator.com