Thread: 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
<p dir="ltr">Peter G is sitting near me and reminded me that this issue came up in the past. Iirc the conclusion then isthat we're calling memcpy where the source and destination pointers are sometimes identical. Tom decided there was reallyno realistic architecture where that wouldn't work. We're not calling it on overlapping nonidentical pointers. <divclass="gmail_quote">On Feb 17, 2013 2:22 PM, "Andres Freund" <<a href="mailto:andres@2ndquadrant.com">andres@2ndquadrant.com</a>>wrote:<br type="attribution" /><blockquote class="gmail_quote"style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> ==24373== Source and destinationoverlap in strncpy(0x28b892f5, 0x28b892f5, 64)<br /> ==24373== at 0x402A8F2: strncpy (mc_replace_strmem.c:477)<br/> ==24373== by 0x7D563F: namestrcpy (name.c:221)<br /> ==24373== by 0x46DF31: TupleDescInitEntry(tupdesc.c:473)<br /> ==24373== by 0x889EC3: resolve_polymorphic_tupdesc (funcapi.c:573)<br /> ==24373== by 0x889873: internal_get_result_type (funcapi.c:322)<br /> ==24373== by 0x8896A2: get_expr_result_type (funcapi.c:235)<br/> ==24373== by 0x594577: addRangeTableEntryForFunction (parse_relation.c:1206)<br /> ==24373== by0x57D81E: transformRangeFunction (parse_clause.c:550)<br /> ==24373== by 0x57DBE1: transformFromClauseItem (parse_clause.c:658)<br/> ==24373== by 0x57CF01: transformFromClause (parse_clause.c:120)<br /> ==24373== by 0x54F9A5:transformSelectStmt (analyze.c:925)<br /> ==24373== by 0x54E4E9: transformStmt (analyze.c:242)<br /><br /> ==24373==Source and destination overlap in memcpy(0x546abc0, 0x546abc0, 24)<br /> ==24373== at 0x402B930: memcpy (mc_replace_strmem.c:883)<br/> ==24373== by 0x853BAB: uniqueentry (tsvector.c:127)<br /> ==24373== by 0x8541A5: tsvectorin(tsvector.c:262)<br /> ==24373== by 0x888781: InputFunctionCall (fmgr.c:1916)<br /> ==24373== by 0x888A7D:OidInputFunctionCall (fmgr.c:2047)<br /> ==24373== by 0x59B6D7: stringTypeDatum (parse_type.c:592)<br /> ==24373== by 0x580E14: coerce_type (parse_coerce.c:303)<br /> ==24373== by 0x580AD4: coerce_to_target_type (parse_coerce.c:101)<br/> ==24373== by 0x58B802: transformTypeCast (parse_expr.c:2222)<br /> ==24373== by 0x587484:transformExprRecurse (parse_expr.c:208)<br /> ==24373== by 0x587156: transformExpr (parse_expr.c:116)<br /> ==24373== by 0x5975CC: transformTargetEntry (parse_target.c:94)<br /><br /> I didn't check out the tsvector case but the<br/> resolve_polymorphic_tupdesc/TupleDescInitEntry is clearly bogusly coded.<br /><br /> Do we care? strncpy'ing a stringover itself isn't defined...<br /><br /> Greetings,<br /><br /> Andres Freund<br /><br /> --<br /> Andres Freund <a href="http://www.2ndQuadrant.com/" target="_blank">http://www.2ndQuadrant.com/</a><br /> PostgreSQLDevelopment, 24x7 Support, Training & Services<br /><br /><br /> --<br /> Sent via pgsql-hackers mailing list(<a href="mailto:pgsql-hackers@postgresql.org">pgsql-hackers@postgresql.org</a>)<br /> To make changes to your subscription:<br/><a href="http://www.postgresql.org/mailpref/pgsql-hackers" target="_blank">http://www.postgresql.org/mailpref/pgsql-hackers</a><br/></blockquote></div>
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@2ndquadrant.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@2ndquadrant.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@cybertec.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@sss.pgh.pa.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@sss.pgh.pa.us> schrieb: >Andres Freund <andres@2ndquadrant.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@sss.pgh.pa.us> schrieb: >Boszormenyi Zoltan <zb@cybertec.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 oneto the tree? --- Please excuse brevity and formatting - I am writing this on my mobile phone.
On 17 February 2013 18:52, anarazel@anarazel.de <andres@anarazel.de> wrote: > You already need a suppression file to use valgrind sensibly, its easy enough to add it there. Perhaps we should add oneto 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.geoghegan86@gmail.com> schrieb: >On 17 February 2013 18:52, anarazel@anarazel.de <andres@anarazel.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 applyto 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@anarazel.de <andres@anarazel.de> wrote: > What patch are you talking about? I have no knowledge about any pending valgrind patches except one I made upstream applyto 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@anarazel.de <andres@anarazel.de> wrote: > > What patch are you talking about? I have no knowledge about any pending valgrind patches except one I made upstream applyto 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