Re: overlapping strncpy/memcpy errors via valgrind

==24373== Source and destination overlap in strncpy(0x28b892f5, 0x28b892f5, 64)
==24373== at 0x402A8F2: strncpy (mc_replace_strmem.c:477)
==24373== by 0x7D563F: namestrcpy (name.c:221)
==24373== by 0x46DF31: TupleDescInitEntry (tupdesc.c:473)
==24373== by 0x889EC3: resolve_polymorphic_tupdesc (funcapi.c:573)
==24373== by 0x889873: internal_get_result_type (funcapi.c:322)
==24373== by 0x8896A2: get_expr_result_type (funcapi.c:235)
==24373== by 0x594577: addRangeTableEntryForFunction (parse_relation.c:1206)
==24373== by 0x57D81E: transformRangeFunction (parse_clause.c:550)
==24373== by 0x57DBE1: transformFromClauseItem (parse_clause.c:658)
==24373== by 0x57CF01: transformFromClause (parse_clause.c:120)
==24373== by 0x54F9A5: transformSelectStmt (analyze.c:925)
==24373== by 0x54E4E9: transformStmt (analyze.c:242)

==24373== Source and destination overlap in memcpy(0x546abc0, 0x546abc0, 24)
==24373== at 0x402B930: memcpy (mc_replace_strmem.c:883)
==24373== by 0x853BAB: uniqueentry (tsvector.c:127)
==24373== by 0x8541A5: tsvectorin (tsvector.c:262)
==24373== by 0x888781: InputFunctionCall (fmgr.c:1916)
==24373== by 0x888A7D: OidInputFunctionCall (fmgr.c:2047)
==24373== by 0x59B6D7: stringTypeDatum (parse_type.c:592)
==24373== by 0x580E14: coerce_type (parse_coerce.c:303)
==24373== by 0x580AD4: coerce_to_target_type (parse_coerce.c:101)
==24373== by 0x58B802: transformTypeCast (parse_expr.c:2222)
==24373== by 0x587484: transformExprRecurse (parse_expr.c:208)
==24373== by 0x587156: transformExpr (parse_expr.c:116)
==24373== by 0x5975CC: transformTargetEntry (parse_target.c:94)

I didn't check out the tsvector case but the
resolve_polymorphic_tupdesc/TupleDescInitEntry is clearly bogusly coded.

Do we care? strncpy'ing a string over itself isn't defined...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Peter G is sitting near me and reminded me that this issue came up in the
past. Iirc the conclusion then is that we're calling memcpy where the
source and destination pointers are sometimes identical. Tom decided there
was really no realistic architecture where that wouldn't work. We're not
calling it on overlapping nonidentical pointers.
On Feb 17, 2013 2:22 PM, "Andres Freund" <andres(at)2ndquadrant(dot)com> wrote:

> ==24373== Source and destination overlap in strncpy(0x28b892f5,
> 0x28b892f5, 64)
> ==24373== at 0x402A8F2: strncpy (mc_replace_strmem.c:477)
> ==24373== by 0x7D563F: namestrcpy (name.c:221)
> ==24373== by 0x46DF31: TupleDescInitEntry (tupdesc.c:473)
> ==24373== by 0x889EC3: resolve_polymorphic_tupdesc (funcapi.c:573)
> ==24373== by 0x889873: internal_get_result_type (funcapi.c:322)
> ==24373== by 0x8896A2: get_expr_result_type (funcapi.c:235)
> ==24373== by 0x594577: addRangeTableEntryForFunction
> (parse_relation.c:1206)
> ==24373== by 0x57D81E: transformRangeFunction (parse_clause.c:550)
> ==24373== by 0x57DBE1: transformFromClauseItem (parse_clause.c:658)
> ==24373== by 0x57CF01: transformFromClause (parse_clause.c:120)
> ==24373== by 0x54F9A5: transformSelectStmt (analyze.c:925)
> ==24373== by 0x54E4E9: transformStmt (analyze.c:242)
>
> ==24373== Source and destination overlap in memcpy(0x546abc0, 0x546abc0,
> 24)
> ==24373== at 0x402B930: memcpy (mc_replace_strmem.c:883)
> ==24373== by 0x853BAB: uniqueentry (tsvector.c:127)
> ==24373== by 0x8541A5: tsvectorin (tsvector.c:262)
> ==24373== by 0x888781: InputFunctionCall (fmgr.c:1916)
> ==24373== by 0x888A7D: OidInputFunctionCall (fmgr.c:2047)
> ==24373== by 0x59B6D7: stringTypeDatum (parse_type.c:592)
> ==24373== by 0x580E14: coerce_type (parse_coerce.c:303)
> ==24373== by 0x580AD4: coerce_to_target_type (parse_coerce.c:101)
> ==24373== by 0x58B802: transformTypeCast (parse_expr.c:2222)
> ==24373== by 0x587484: transformExprRecurse (parse_expr.c:208)
> ==24373== by 0x587156: transformExpr (parse_expr.c:116)
> ==24373== by 0x5975CC: transformTargetEntry (parse_target.c:94)
>
> I didn't check out the tsvector case but the
> resolve_polymorphic_tupdesc/TupleDescInitEntry is clearly bogusly coded.
>
> Do we care? strncpy'ing a string over itself isn't defined...
>
> Greetings,
>
> Andres Freund
>
> --
> Andres Freund http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

On 2013-02-17 15:10:35 +0000, Greg Stark wrote:
> Peter G is sitting near me and reminded me that this issue came up in the
> past. Iirc the conclusion then is that we're calling memcpy where the
> source and destination pointers are sometimes identical. Tom decided there
> was really no realistic architecture where that wouldn't work.

I am not so convinced that that is safe if libc turns that into some
optimized string instructions or even PCMPSTR...

> We're not calling it on overlapping nonidentical pointers.

Yup, the backtrace shows that...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2013-02-17 15:10:35 +0000, Greg Stark wrote:
>> Peter G is sitting near me and reminded me that this issue came up in the
>> past. Iirc the conclusion then is that we're calling memcpy where the
>> source and destination pointers are sometimes identical. Tom decided there
>> was really no realistic architecture where that wouldn't work.

> I am not so convinced that that is safe if libc turns that into some
> optimized string instructions or even PCMPSTR...

What would you envision happening that would be bad? The reason
overlapping source/dest is undefined is essentially that the function is
allowed to copy bytes in an unspecified order. But if the pointers are
the same, then no matter what it does, no individual store will replace
a byte with a different value than it had, and so it's not possible for
the order of operations to matter.

I don't think it's worth contorting the source code to suppress this
complaint. In the case of resolve_polymorphic_tupdesc, for instance,
dodging this warning would require that we not use TupleDescInitEntry to
update the data type of an attribute, but instead have this code know
all about the subsidiary fields that get set from the datatype. I'm not
seeing that as an improvement ...

regards, tom lane

2013-02-17 16:32 keltezéssel, Tom Lane írta:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
>> On 2013-02-17 15:10:35 +0000, Greg Stark wrote:
>>> Peter G is sitting near me and reminded me that this issue came up in the
>>> past. Iirc the conclusion then is that we're calling memcpy where the
>>> source and destination pointers are sometimes identical. Tom decided there
>>> was really no realistic architecture where that wouldn't work.
>> I am not so convinced that that is safe if libc turns that into some
>> optimized string instructions or even PCMPSTR...
> What would you envision happening that would be bad? The reason
> overlapping source/dest is undefined is essentially that the function is
> allowed to copy bytes in an unspecified order. But if the pointers are
> the same, then no matter what it does, no individual store will replace
> a byte with a different value than it had, and so it's not possible for
> the order of operations to matter.

Then, why isn't memcpy() skipped if the source and dest are the same?
It would be a micro-optimization but a valid one.

> I don't think it's worth contorting the source code to suppress this
> complaint. In the case of resolve_polymorphic_tupdesc, for instance,
> dodging this warning would require that we not use TupleDescInitEntry to
> update the data type of an attribute, but instead have this code know
> all about the subsidiary fields that get set from the datatype. I'm not
> seeing that as an improvement ...
>
> regards, tom lane
>
>

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/

Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
> Then, why isn't memcpy() skipped if the source and dest are the same?
> It would be a micro-optimization but a valid one.

No, it'd be more like a micro-pessimization, because the test would be
wasted effort in the vast majority of calls. The *only* reason to do
this would be to shut up valgrind, and that seems annoying.

I wonder if anyone's tried filing a bug against valgrind to say that it
shouldn't complain about this case.

regards, tom lane

On Sun, Feb 17, 2013 at 4:26 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> No, it'd be more like a micro-pessimization, because the test would be
> wasted effort in the vast majority of calls. The *only* reason to do
> this would be to shut up valgrind, and that seems annoying.

In terms of runtime I strongly suspect the effect would be 0 due to
branch prediction.

The effect on the code cleanliness seems like a stronger argument but
I have a hard time getting upset about a single one-line if statement
in namestrcpy. I suspect the argument may have been that we have no
reason to believe namestrcpy is the only place this can happen.

--
greg

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> schrieb:

>Andres Freund <andres(at)2ndquadrant(dot)com> writes:
>> On 2013-02-17 15:10:35 +0000, Greg Stark wrote:
>>> Peter G is sitting near me and reminded me that this issue came up
>in the
>>> past. Iirc the conclusion then is that we're calling memcpy where
>the
>>> source and destination pointers are sometimes identical. Tom decided
>there
>>> was really no realistic architecture where that wouldn't work.
>
>> I am not so convinced that that is safe if libc turns that into some
>> optimized string instructions or even PCMPSTR...
>
>What would you envision happening that would be bad?

Afair some of the optimized instructions (like movdqa) don't necessarily work if source an target are in the same location. Not sure about it bit I wouldn't want to depend on it.

Andres

---
Please excuse brevity and formatting - I am writing this on my mobile phone.

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> schrieb:

>Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
>> Then, why isn't memcpy() skipped if the source and dest are the same?
>> It would be a micro-optimization but a valid one.
>
>No, it'd be more like a micro-pessimization, because the test would be
>wasted effort in the vast majority of calls. The *only* reason to do
>this would be to shut up valgrind, and that seems annoying.
>
>I wonder if anyone's tried filing a bug against valgrind to say that it
>shouldn't complain about this case.

You already need a suppression file to use valgrind sensibly, its easy enough to add it there. Perhaps we should add one to the tree?

---
Please excuse brevity and formatting - I am writing this on my mobile phone.

On 17 February 2013 18:52, anarazel(at)anarazel(dot)de <andres(at)anarazel(dot)de> wrote:
> You already need a suppression file to use valgrind sensibly, its easy enough to add it there. Perhaps we should add one to the tree?

Perhaps you should take the time to submit a proper Valgrind patch
first. The current approach of applying the patch that Noah Misch
originally wrote (but did not publicly submit, iirc) on an ad-hoc
basis isn't great. That is what you've done here, right?

--
Regards,
Peter Geoghegan

Peter Geoghegan <peter(dot)geoghegan86(at)gmail(dot)com> schrieb:

>On 17 February 2013 18:52, anarazel(at)anarazel(dot)de <andres(at)anarazel(dot)de>
>wrote:
>> You already need a suppression file to use valgrind sensibly, its
>easy enough to add it there. Perhaps we should add one to the tree?
>
>Perhaps you should take the time to submit a proper Valgrind patch
>first. The current approach of applying the patch that Noah Misch
>originally wrote (but did not publicly submit, iirc) on an ad-hoc
>basis isn't great. That is what you've done here, right?

What patch are you talking about? I have no knowledge about any pending valgrind patches except one I made upstream apply to make pg inside valgrind work on amd64.

Andres

---
Please excuse brevity and formatting - I am writing this on my mobile phone.

On 17 February 2013 19:39, anarazel(at)anarazel(dot)de <andres(at)anarazel(dot)de> wrote:
> What patch are you talking about? I have no knowledge about any pending valgrind patches except one I made upstream apply to make pg inside valgrind work on amd64.

Noah wrote a small patch, which he shared with me privately, which
added Valgrind hook macros to aset.c and mcxt.c. The resulting
Valgrind run threw up some things that were reported publicly [1]. I
documented much of his work on the wiki. I was under the impression
that this was the best way to get Valgrind to work with Postgres
(apparently there were problems with many false positives otherwise).

[1] http://www.postgresql.org/message-id/20110312133224.GA7833@tornado.gateway.2wire.net

--
Regards,
Peter Geoghegan

On 2013-02-17 19:52:16 +0000, Peter Geoghegan wrote:
> On 17 February 2013 19:39, anarazel(at)anarazel(dot)de <andres(at)anarazel(dot)de> wrote:
> > What patch are you talking about? I have no knowledge about any pending valgrind patches except one I made upstream apply to make pg inside valgrind work on amd64.
>
> Noah wrote a small patch, which he shared with me privately, which
> added Valgrind hook macros to aset.c and mcxt.c. The resulting
> Valgrind run threw up some things that were reported publicly [1]. I
> documented much of his work on the wiki. I was under the impression
> that this was the best way to get Valgrind to work with Postgres
> (apparently there were problems with many false positives otherwise).
>
> [1] http://www.postgresql.org/message-id/20110312133224.GA7833@tornado.gateway.2wire.net

Nice, I wasn't aware of that work. I always wanted to add that
instrumentation but never got arround to it.
PG runs without problems for me with the exception of some warnings that
I suppress.
Would be nice to get that upstream...

> For reasons that have yet to be ascertained, it is necessary to run the
> regression tests with autovacuum = 'off'. Otherwise, Postgres will segfault
> within an autovacuum worker's elog() call.

That's the bug I was referring to, its fixed at least in svn. It failed
in far more places than that, basically everywhere an instruction that
required the stack to be properly aligned was executed.
The problem was that valgrind didn't align the new stack properly after
a fork if the fork was executed inside a signal handler. Which pg
happens to do...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services