Thread: fix memcpy() overlap

Another error reported by valgrind on the postmaster (& children) is
the following:

==30568== Source and destination overlap in memcpy(0xBFFFEA30, 0xBFFFEA30, 24)
==30568==    at 0x40024224: memcpy (mac_replace_strmem.c:93)
==30568==    by 0x82081F3: set_var_from_var (numeric.c:2732)
==30568==    by 0x820BDF7: power_var_int (numeric.c:4572)
==30568==    by 0x820A74E: exp_var (numeric.c:4180)
==30568==    by 0x820BBF4: power_var (numeric.c:4514)
==30568==    by 0x8205AA0: numeric_power (numeric.c:1580)
==30568==    by 0x8141814: ExecMakeFunctionResult (execQual.c:907)
==30568==    by 0x8141FD5: ExecEvalFunc (execQual.c:1214)
==30568==    by 0x8143E61: ExecEvalExpr (execQual.c:2253)
==30568==    by 0x8141323: ExecEvalFuncArgs (execQual.c:678)
==30568==    by 0x81414A0: ExecMakeFunctionResult (execQual.c:736)
==30568==    by 0x8141FD5: ExecEvalFunc (execQual.c:1214)

[ triggered by one of the queries in the numeric regression test ]

I don't know of a memcpy() implementation that would actually bail out
if called with two equal pointers, but perhaps there is one in
existence somewhere. The attached patch (not yet applied) avoids this
case by returning early from set_var_from_var() if asked to assign a
numeric to itself. That also means we can skip a few memcpy()s and a
palloc(). The other way to fix this would be to just use memmove()
instead of memcpy() here: anyone have a preference?

Unless anyone objects, I'll apply this patch tonight.

-Neil

Neil Conway <neilc@samurai.com> writes:
> I don't know of a memcpy() implementation that would actually bail out
> if called with two equal pointers, but perhaps there is one in
> existence somewhere.

This isn't a bug, and I see no reason to clutter the code just to shut
up valgrind.

            regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:
> This isn't a bug

If that's the case I'm content with not changing the code, but I'd
rather not just take it on faith: can you point me to some authority
that says two objects with the same address do not "overlap"?

(I checked C99 (draft 843) and while it references overlapping objects
several times, it never precisely defines the term.)

> I see no reason to clutter the code just to shut up valgrind.

Sure -- it is either our bug, or a bug in valgrind.

-Neil

On Mon, 2 Feb 2004, Tom Lane wrote:

> Neil Conway <neilc@samurai.com> writes:
> > I don't know of a memcpy() implementation that would actually bail out
> > if called with two equal pointers, but perhaps there is one in
> > existence somewhere.
>
> This isn't a bug, and I see no reason to clutter the code just to shut
> up valgrind.

Isn't memcpy on overlapping (even entirely overlapping) buffers undefined
behavior unless the count is 0?

On Mon, Feb 02, 2004 at 01:23:15PM -0800, Stephan Szabo wrote:
>
> Isn't memcpy on overlapping (even entirely overlapping) buffers undefined
> behavior unless the count is 0?

The C standard says: "If copying takes place between objects that overlap,
the behaviour is undefined". SUSv3 says the same.

To my understanding this also includes copying something onto itself.

--
                                Michael van Elst
Internet: mlelstv@serpens.de
                                "A potential Snark may lurk in every tree."

Stephan Szabo wrote:

>On Mon, 2 Feb 2004, Tom Lane wrote:
>
>
>
>>Neil Conway <neilc@samurai.com> writes:
>>
>>
>>>I don't know of a memcpy() implementation that would actually bail out
>>>if called with two equal pointers, but perhaps there is one in
>>>existence somewhere.
>>>
>>>
>>This isn't a bug, and I see no reason to clutter the code just to shut
>>up valgrind.
>>
>>
>
>Isn't memcpy on overlapping (even entirely overlapping) buffers undefined
>behavior unless the count is 0?
>
>
>

On my Linux box the man page says:

DESCRIPTION
       The  memcpy()  function  copies  n bytes from memory area src to
memory
       area dest.  The memory areas may not overlap.  Use  memmove(3)
if  the
       memory areas do overlap.

cheers

andrew

Stephan Szabo <sszabo@megazone.bigpanda.com> writes:
> On Mon, 2 Feb 2004, Tom Lane wrote:
>> This isn't a bug, and I see no reason to clutter the code just to shut
>> up valgrind.

> Isn't memcpy on overlapping (even entirely overlapping) buffers undefined
> behavior unless the count is 0?

The reason that the spec describes overlapped memcpy as undefined is
that it does not want to restrict which direction the copy occurs in
(proceeding from lower to higher memory addresses or vice versa).
memmove is constrained to do the copy in the direction that will avoid
failure when the source and destination partially overlap.  But memcpy
is expected to do whichever is fastest on the particular architecture,
without concern for possible overlap.  (Offhand I've never heard of a
machine where memcpy doesn't work lower-to-higher, but maybe there is
one.)

However, when the source and destination buffers are the same, it does
not matter which direction you copy in; you are picking up and putting
down the same bytes at the same addresses no matter what order you
process the bytes in.  There is no implementation in existence that will
produce an unwanted result.

If you want to argue about dependencies on implementation details that
are theoretically undefined according to the ANSI C spec, we have tons
more beyond this one.  F'r instance, depending on twos-complement
arithmetic is illegal per spec also ...

            regards, tom lane

Tom Lane wrote:
> Stephan Szabo <sszabo@megazone.bigpanda.com> writes:
> > On Mon, 2 Feb 2004, Tom Lane wrote:
> >> This isn't a bug, and I see no reason to clutter the code just to shut
> >> up valgrind.
>
> > Isn't memcpy on overlapping (even entirely overlapping) buffers undefined
> > behavior unless the count is 0?
>
> The reason that the spec describes overlapped memcpy as undefined is
> that it does not want to restrict which direction the copy occurs in
> (proceeding from lower to higher memory addresses or vice versa).
> memmove is constrained to do the copy in the direction that will avoid
> failure when the source and destination partially overlap.  But memcpy
> is expected to do whichever is fastest on the particular architecture,
> without concern for possible overlap.  (Offhand I've never heard of a
> machine where memcpy doesn't work lower-to-higher, but maybe there is
> one.)
>
> However, when the source and destination buffers are the same, it does
> not matter which direction you copy in; you are picking up and putting
> down the same bytes at the same addresses no matter what order you
> process the bytes in.  There is no implementation in existence that will
> produce an unwanted result.
>
> If you want to argue about dependencies on implementation details that
> are theoretically undefined according to the ANSI C spec, we have tons
> more beyond this one.  F'r instance, depending on twos-complement
> arithmetic is illegal per spec also ...

Isn't memmove() for overlaping regions?  That's what my BSD manual page
says.

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Isn't memmove() for overlaping regions?  That's what my BSD manual page
> says.

If we want to get rid of the valgrind warning, a simpler patch would be
to substitute memmove for memcpy in that particular numeric.c subroutine
(at least, I assume this will shut valgrind up).  I could live with that;
it seems a less intrusive fix than a special-case test that will hardly
ever trigger.

            regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:
> If we want to get rid of the valgrind warning, a simpler patch would
> be to substitute memmove for memcpy

I've made this change and committed it to HEAD.

-Neil