GNOME Bugzilla – Bug 710989
Replace most uses of strcpy()
Last modified: 2014-02-07 14:46:07 UTC
Created attachment 258270 [details] [review] 0001-Replace-most-uses-of-strcpy-with-g_strlcopy.patch As with bug #710787, this patch replaces strcpy() with g_strlcpy() to avoid buffer overruns. It leaves some strcpy() uses that are more awkward. They would need a little more care and testing.
Created attachment 258271 [details] [review] 0002-Replace-some-more-uses-of-strcpy-with-g_strlcpy.patch This patch replaces a couple more uses of strcpy(), but I find the char arrays of size 1 odd, so I guess there is some other cleverness going on there.
Created attachment 258272 [details] [review] 0001-Replace-most-uses-of-strcpy-with-g_strlcopy.patch Uploading again. I seem to have created the previous patch in the middle of a rebase.
Created attachment 258273 [details] [review] 0002-Replace-some-more-uses-of-strcpy-with-g_strlcpy.patch Uploading again. I seem to have created the previous patch in the middle of a rebase. Again, this one involves some odd char arrays of length 1.
Thanks a bunch for trudging through this. Patches look good as they are. Committed for E-D-S 3.11.2 and 3.10.2: https://git.gnome.org/browse/evolution-data-server/commit/?id=61bf9fb860ecb4c6ace37cf3414d68d298ec26cf https://git.gnome.org/browse/evolution-data-server/commit/?h=gnome-3-10&id=1a6efe8419789661bf5bf47d2d59f9b9e978d77f
Thanks for pushing. But didn't you find the second one a bit odd, with those char[1] arrays?
This has a regression, bug #720751. I guess it's what you meant with those char[1] arrays, the sizeof(char[1]) is 1, thus you told g_strlcpy() to copy up to 1 letter, which breaks things.
Thanks for fixing that. Although it probably works now again, it's still a bit strange: typedef struct _CamelFlag { struct _CamelFlag *next; gchar name[1]; /* name allocated as part of the structure */ } CamelFlag; CamelFlag *tmp; ... tmp_len = sizeof (*tmp) + strlen (name); tmp = g_malloc (tmp_len); g_strlcpy (tmp->name, name, strlen (name) + 1); So the CamelFlag.name gchar array can actually have more than 1 character, but only because it's inside a struct that was allocated with too much space for just the struct itself. I wonder why a simple "const gchar* name" field wouldn't be OK. I can't see any particular reason that this trick is needed.
I'm guessing it was done that way for "efficiency", though it's a pretty unsafe practice. Highly prone to memory corruption. Probably also why the struct embeds its own linked list pointer. If I had time to rewrite this I'd just make it a GList or GPtrArray of allocated strings.
(In reply to comment #7) > I wonder why a simple "const gchar* name" field wouldn't be OK. I can't see any > particular reason that this trick is needed. It's because 'name[1]' tells the compiler, that the 'name' value starts at the end of the structure (there should not be any other variable after this one in the structure definition), while generic '*name' points to anywhere, not at the end of the structure. Actual reasons for this are also hidden to me, I would prefer to see there a GSList instead too, but I was never brave enough to move the idea into the action.