[Bug 269] New: some code doesn't follow glib recomendations
https://bugzilla.balabit.com/show_bug.cgi?id=269 Summary: some code doesn't follow glib recomendations Product: syslog-ng Version: 3.5.x Platform: PC OS/Version: Linux Status: NEW Severity: major Priority: unspecified Component: syslog-ng AssignedTo: bazsi@balabit.hu ReportedBy: iippolitov@gmail.com Type of the Report: bug Estimated Hours: 0.0 I'm using rhel 5.9 and syslog-ng 3.5.3. tests/unit/test_nvtable fails on line 69. It is a know bug: https://bugzilla.redhat.com/show_bug.cgi?id=714409#c4 . As per https://bugzilla.redhat.com/show_bug.cgi?id=716447 :
From the glib2 documentation: ... Note that neither keys nor values are copied when inserted into the GHashTable, so they must exist for the lifetime of the GHashTable. This means that the use of static strings is OK, but temporary strings (i.e. those created in buffers and those returned by GTK+ widgets) should be copied with g_strdup() before being inserted.
If keys or values are dynamically allocated, you must be careful to ensure that they are freed when they are removed from the GHashTable, and also when they are overwritten by new insertions into the GHashTable. It is also not advisable to mix static strings and dynamically-allocated strings in a GHashTable, because it then becomes difficult to determine whether the string should be freed. ... The code in lib/nv_table.c uses a pointer to a string that is changed during a program execution to insert values into GHashTable. This causes some unit tests to fail at least in rhel5. nv_registry_alloc_handle was obviously fixed to g_strdup() string from function parameters, while nv_registry_add_alias still uses a pointer the function gets as a parameter: Here is a workaround: void nv_registry_add_alias(NVRegistry *self, NVHandle handle, const gchar *alias) { g_static_mutex_lock(&nv_registry_lock); - g_hash_table_insert(self->name_map, (gchar*)alias, GUINT_TO_POINTER((glong) handle)); + gchar * st_alias = g_strdup(alias); + g_hash_table_insert(self->name_map, st_alias, GUINT_TO_POINTER((glong) handle)); g_static_mutex_unlock(&nv_registry_lock); } AFAIK, g_hash_table_insert is used widely around syslog-ng code. Could someone please look it through and ensure that any value added into gHashTable is not changed during hashtable lifetime? Feel free to contact me via email or gtalk to find out any details I might miss. Thanks in advance. -- Configure bugmail: https://bugzilla.balabit.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.
https://bugzilla.balabit.com/show_bug.cgi?id=269 --- Comment #1 from Balazs Scheidler <bazsi@balabit.hu> 2014-01-27 08:10:19 --- Thanks for the diagnosis. Im aware of this ghashtable requirement, its probably only the test program that is affected. Ill check the rest. -- Configure bugmail: https://bugzilla.balabit.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.
https://bugzilla.balabit.com/show_bug.cgi?id=269 --- Comment #2 from Igor Ippolitov <iippolitov@gmail.com> 2014-01-31 10:05:30 --- Created an attachment (id=89) --> (https://bugzilla.balabit.com/attachment.cgi?id=89) a patch for nvtable.c and .h files. Here is a more neat and tested solution for nvtable.c test problem. Seems it doesn't break anything and solves a problem with tests ;) Another way of implementing aliases is passing an additional parameter into allocate_handle function with a desired handle. I suppose, we can close this bug if you are not going to do anything about it later. -- Configure bugmail: https://bugzilla.balabit.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.
https://bugzilla.balabit.com/show_bug.cgi?id=269 --- Comment #3 from Balazs Scheidler <bazsi@balabit.hu> 2014-01-31 14:08:43 --- what about this patch instead? it delegates the freeing of the key to GHashTable and avoids having to store aliases in a separate array. the unit test seems to run fine, but I didn't do very thorough testing/review. feedback is appreciated. diff --git a/lib/nvtable.c b/lib/nvtable.c index 504bebb..8462f76 100644 --- a/lib/nvtable.c +++ b/lib/nvtable.c @@ -102,7 +102,7 @@ void nv_registry_add_alias(NVRegistry *self, NVHandle handle, const gchar *alias) { g_static_mutex_lock(&nv_registry_lock); - g_hash_table_insert(self->name_map, (gchar *) alias, GUINT_TO_POINTER((glong) handle)); + g_hash_table_insert(self->name_map, g_strdup(alias), GUINT_TO_POINTER((glong) handle)); g_static_mutex_unlock(&nv_registry_lock); } @@ -124,7 +124,7 @@ nv_registry_new(const gchar **static_names) NVRegistry *self = g_new0(NVRegistry, 1); gint i; - self->name_map = g_hash_table_new_full(g_str_hash, g_str_equal, NULL, NULL); + self->name_map = g_hash_table_new_full(g_str_hash, g_str_equal, g_free, NULL); self->names = g_array_new(FALSE, FALSE, sizeof(NVHandleDesc)); for (i = 0; static_names[i]; i++) { @@ -136,10 +136,6 @@ nv_registry_new(const gchar **static_names) void nv_registry_free(NVRegistry *self) { - gint i; - - for (i = 0; i < self->names->len; i++) - g_free(g_array_index(self->names, NVHandleDesc, i).name); g_array_free(self->names, TRUE); g_hash_table_destroy(self->name_map); g_free(self); -- Configure bugmail: https://bugzilla.balabit.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.
https://bugzilla.balabit.com/show_bug.cgi?id=269 --- Comment #4 from Igor Ippolitov <iippolitov@gmail.com> 2014-01-31 14:19:05 --- In nv_registry_alloc_handle there is a check for self->name_map length: it should be less than 65535. If you store aliases in the same array with names you shrink available name quantity per table. I don't know what's the average use of an nv_table and if adding aliases into the same namespace can break anything, so it's up to you. The code itself is OK and seems to be fully functional in my case (I don't perform full functional tests, I only test some things I really need). -- Configure bugmail: https://bugzilla.balabit.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.
https://bugzilla.balabit.com/show_bug.cgi?id=269 --- Comment #5 from Balazs Scheidler <bazsi@balabit.hu> 2014-02-01 07:34:51 --- (In reply to comment #4)
In nv_registry_alloc_handle there is a check for self->name_map length: it should be less than 65535. If you store aliases in the same array with names you shrink available name quantity per table.
that check is not against name_map, but rather against "names", where aliases are not inserted, so this limit is not changed at all.
I don't know what's the average use of an nv_table and if adding aliases into the same namespace can break anything, so it's up to you. The code itself is OK and seems to be fully functional in my case (I don't perform full functional tests, I only test some things I really need).
aliases should be in the same namespace, but shouldn't be counted in the limit. but they are not. I'm committing this patch. I'm adding a signed-off-by tag on your behalf, please let me know if it's not ok for you. The signed-off-by tag indicates that you rightfully submitted and contributed the patch, it is explained here https://www.kernel.org/doc/Documentation/SubmittingPatches Thanks. -- Configure bugmail: https://bugzilla.balabit.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.
https://bugzilla.balabit.com/show_bug.cgi?id=269 --- Comment #6 from Balazs Scheidler <bazsi@balabit.hu> 2014-02-01 07:39:49 --- this is where I've integrated it in the 3.6 branch commit a8f4b42142593b911457cabab5acbcf5157b8371 Author: Igor Ippolitov <iippolitov@gmail.com> Date: Sat Feb 1 07:35:00 2014 +0100 nvtable: fix an NV registry corruption in the unit tests nv_registry_add_alias() used the passed in pointer to the alias verbatim, without copying it. However the unit test program generated alias names dynamically, e.g. editing the memory area that was used to store the hash table key. Since this wasn't intuitive at all, this patch changes the code so that the key in the NV registry is now always duplicated before insertion. Signed-off-by: Igor Ippolitov <iippolitov@gmail.com> Signed-off-by: Balazs Scheidler <bazsi@balabit.hu> -- Configure bugmail: https://bugzilla.balabit.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.
https://bugzilla.balabit.com/show_bug.cgi?id=269 Igor Ippolitov <iippolitov@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution| |FIXED Status|NEW |RESOLVED --- Comment #7 from Igor Ippolitov <iippolitov@gmail.com> 2014-02-04 08:44:26 --- Ok, great. Thanks. -- Configure bugmail: https://bugzilla.balabit.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.
participants (1)
-
bugzilla@bugzilla.balabit.com