Re: strncpy is not a safe version of strcpy

On Sat, Nov 16, 2013 at 4:09 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>wrote:

> David Rowley escribió:
> > On Fri, Nov 15, 2013 at 12:33 PM, Tomas Vondra <tv(at)fuzzy(dot)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.

Regards

David Rowley

> --
> Álvaro Herrera http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>