GNOME Bugzilla – Bug 168371
GOK should use consistent memory/string APIs
Last modified: 2006-03-31 20:35:13 UTC
GOK's sources use a mix and match of alloc/free/string APIs. While it's appropriate to use both libxml2 and glib (and CORBA-string) apis in places where glib and libxml2 are required for other reasons, there are plenty of places where we are either mismatching (using g_free where xmlFree would be better), or using more 'primitive' API such as malloc+memcpy in place of strdup, etc. grep the source code for 'alloc', 'memcpy', 'strcpy', etc. see bug 168218 for an example, in the latter comments.
Let me raise priority for the following reason: While translating gok to Russian I've met the following problem: Access method names are displayed incorrectly in Preferences dialon on "Access method" tab in combobox. I've found the reason of such bug - the GokAccessMethod stucture uses strings of very small fixed width, for example pDisplayName is gchar[51]. Of course, it's not enough to fit translated string. And gok uses strcpy to assign values to such strings. Such programming style is very dangerous. The half-solution is to warn translators about such thing and force them usage of short strings, but I don't think it appropriate. The attached patch decreases number of strcpy calls in gok code from 118 to 12 and also fixes some problems in sprintf. I think it's required to make gok code safer. The last 12 cases are most complicated but they should be dropped also. Until this problem will be fixed I suggest to increase lenght of fixed strings.
Created attachment 51629 [details] [review] patch
Thanks Nickolay. It will take some time to review such a complex and potentially high-risk patch.
I have reviewed the patch and do not see any errors. There are some very nice improvements here. Thanks very much for this work! Nickolay,I am going to give 1 half of a commit approval. (Bill is comaintainer) One thing... do you think there would ever be a need for a 1000 character action name? Fundamentally, I guess we shouldn't restrict translation length.
Bill, most of the changes are straighforward and repetitive. Perhaps more precarious is the change to use g_strsplit when iterating through tokens (latter part of patch)... but again it _looks_ like an improvement, at least in source clarity.
Comment on attachment 51629 [details] [review] patch fine except for the change in the action loop from 100 max to 1000.
David: the loop doesn't create 1000 character action names, it searches for up to 1000 actions. Doesn't seem sensible to me, I'd leave the current limit of 100, so please don't apply this part of the patch. In other ways it's clearly an improvement. Don't know where or why the odd g_malloc+strdup pattern came from, that was clearly wrong. However the patch is not complete, it fails to modify gok-scanner.h. As such I think we should not apply until we address the problem that originally caused NIkolay's trouble. I don't think any of our structs should have fixed-length char buffers, they should all be pointers to g_strdup'ed text in all likelihood.
Ok, thanks for such quick review. This patch is definitely not for now, it breaks strings and really big. I agree we should wait then with it. While then I'll try to solve remaining problem and submit more consistent patch.
Thanks Bill. Nickolay, thanks for looking into the struct buffers. Please post again when you can. Sorry I didn't catch that issue.
Nickolay, should we just make the fixed-length strings twice as long for the moment (ask the release-team for permission), since it's breaking translation?
I think yes, the longest strings are in Bengali translation, but 100 for access method name and 400 for access method description should be enough even for it.
Created attachment 52885 [details] [review] Updated patch Should solve almost all problems with string memory management.
Hey guys, ping, what about committing this patch?
Thanks Nickolay. Do you have an updated patch against head?
Already attached patch applies with little fuzz but cleanly, so I think there is no sense to update it.
Thanks for all your work and patience Nickolay. Please commit.
Cool, thanks to all for reviews, patch is committed.