Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> Okay, here is a new patch with improved implementations of namecpy() and
> namestrcpy(). I didn't see any other places that relied on the
> zero-filling behavior of strncpy().
I've looked through this patch, and I concur with your conclusion that
noplace else is depending on zero-fill, with the exception of the one
place in pgstat.c that David already noted. But the issue there is only
that valgrind might bitch about send()'ing undefined bytes, and ISTM
that the existing suppressions in our valgrind.supp should already
handle it, since we already have other pgstat messages with pad bytes.
However I do see one remaining nit to pick, in CreateInitDecodingContext:
/* register output plugin name with slot */
SpinLockAcquire(&slot->mutex);
- StrNCpy(NameStr(slot->data.plugin), plugin, NAMEDATALEN);
+ namestrcpy(&slot->data.plugin, plugin);
SpinLockRelease(&slot->mutex);
This is already a pro-forma violation of our rule about "only
straight-line code inside a spinlock". Now I'm not terribly concerned
about that right now, and the patch as it stands is only changing things
cosmetically. But if you modify namestrcpy to do pg_mbcliplen then all
of a sudden there is a whole lot of code that could get reached within
the spinlock, and I'm not a bit happy about that prospect.
The least-risk fix would be to namestrcpy() into a local variable
and then just use a plain memcpy() inside the spinlock. There might
be better ways if we're willing to make assumptions about what the
plugin name can be. For that matter, do we really need a spinlock
here at all? Why is the plugin name critical but the rest of the
slot not?
BTW, while we're here I think we ought to change namecpy and namestrcpy
to return void (no caller checks their results) and drop their checks
for null-pointer inputs. AFAICS a null pointer would be a caller bug in
every case, and if it isn't, why is failing to initialize the
destination an OK outcome? I find the provisions for nulls in namestrcmp
pretty useless too, although it looks like at least some thought has
been spent there.
I think this is committable once these points are addressed.
regards, tom lane