On Sat, Mar 12, 2011 at 12:44:29PM -0500, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > A suitably-instrumented run of "make installcheck-world" under valgrind turned
> > up a handful of memory-related bugs:
>
> Hmm, interesting work, but I don't think I believe in the necessity for
> this kluge:
>
> > + else if (attributeName != &(att->attname))
> > + namestrcpy(&(att->attname), attributeName);
>
> The rules against overlapping memcpy/strcpy's source and destination are
> meant to cover the case of partial overlap; I find it hard to imagine an
> implementation that will mess up when the source and destination are
> identical. If we did think it was important to avoid this situation I
> would rather find another way, like modifying the caller. Likewise
> the other changes to avoid no-op memcpy's do not appear to me to be
> bugs, though possibly they might save enough cycles to be worth doing
> anyway.
I also find it hard to imagine an implementation that needs these changes to
produce correct behavior. Avoiding undefined behavior has intrinsic value, but
perhaps we do get greater value from the existing code.
> > ! stats->attrtype = (Form_pg_type) palloc(sizeof(FormData_pg_type));
> > ! memcpy(stats->attrtype, GETSTRUCT(typtuple), sizeof(FormData_pg_type));
> > ...
> > ! stats->attrtype = (Form_pg_type) palloc(TYPE_FIXED_PART_SIZE);
> > ! memcpy(stats->attrtype, GETSTRUCT(typtuple), TYPE_FIXED_PART_SIZE);
>
> I wonder whether we should instead fix this by copying the correct tuple
> length.
Seems like a step in the wrong direction. We only use typlen and typbyval
beyond the immediate context.