> On Fri, Nov 15, 2013 at 12:33 PM, Tomas Vondra <tv@fuzzy.cz> wrote:
> > Be careful with 'Name' data type - it's not just a simple string buffer. > > AFAIK it needs to work with hashing etc. so the zeroing is actually needed > > here to make sure two values produce the same result. At least that's how > > I understand the code after a quick check - for example this is from the > > same jsonfuncs.c you mentioned: > > > > memset(fname, 0, NAMEDATALEN); > > strncpy(fname, NameStr(tupdesc->attrs[i]->attname), NAMEDATALEN); > > hashentry = hash_search(json_hash, fname, HASH_FIND, NULL); > > > > So the zeroing is on purpose, although if strncpy does that then the > > memset is probably superflous.
This code should probably be using namecpy(). Note namecpy() doesn't memset() after strncpy() and has survived the test of time, which strongly suggests that the memset is indeed superfluous.
I went on a bit of a strncpy cleanup rampage this morning and ended up finding quite a few places where strncpy is used wrongly.
I'm not quite sure if I have got them all in this patch, but I' think I've got the obvious ones at least.
For the hash_search in jsconfuncs.c after thinking about it a bit more... Can we not just pass the attname without making a copy of it? I see keyPtr in hash_search is const void * so it shouldn't get modified in there. I can't quite see the reason for making the copy.
Attached is a patch with various cleanups where I didn't like the look of the strncpy. I didn't go overboard with this as I know making this sort of small changes all over can be a bit scary and I thought maybe it would get rejected on that basis.
I also cleaned up things like strncpy(dest, src, strlen(src)); which just seems a bit weird and I'm failing to get my head around why it was done. I replaced these with memcpy instead, but they could perhaps be a plain old strcpy.
