On Sat, Nov 16, 2013 at 12:53:10PM +1300, David Rowley wrote:
> 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.
+1 for the goal of this patch. Another commit took care of your jsonfuncs.c
concerns, and the patch for CVE-2014-0065 fixed several of the others. Plenty
remain, though.
> 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.
I suggest preparing one or more patches that focus on the cosmetic-only
changes, such as strncpy() -> memcpy() when strncpy() is guaranteed not to
reach a NUL byte. With that noise out of the way, it will be easier to give
the rest the attention it deserves.
Thanks,
nm
--
Noah Misch
EnterpriseDB http://www.enterprisedb.com