GNOME Bugzilla – Bug 665716
rest_params_add should use g_hash_table_replace
Last modified: 2014-08-26 09:17:31 UTC
/*rest_proxy_call_add_param: * Add a query parameter called @param with the string value @value to the call. * If a parameter with this name already exists, the new value will replace the * old. */ The notation of rest_proxy_call_add_param says, the new value will replace the old, so the rest_parmas_add should use g_hash_table_replace to add new parm. This is useful when we sent some requests with 'page' parameter, in this scenario, when we want to retrieve the data of 'next page' or 'previous page', we just need to reset the 'page' parameter.
The only difference between _insert and _replace is whether the old key or new key gets freed if there is already an entry with that key. As the internal hash table used by RestParams doesn't free the key itself, this is a moot point. (I suspect you are looking at an older codebase before RestParam landed)
The last commit of the codebase I use is 'Thu Nov 10'. Two differences between _insert and _replace, another one is g_hash_table_insert will not reset the key point if the key was already exist. (It will destroy the old value.) g_hash_table_insert_node: if (keep_new_key) hash_table->keys[node_index] = key; Although I don't think this 'if' is necessary, but it cause this bug. My glib2 version is glib-2.31.2. In rest_params_add g_hash_table_insert (hash, (gpointer) rest_param_get_name (param), param); the key is point to param->name, after insert another same key, the old value would be destroyed, so the key will point to a wide address. (In reply to comment #1) > The only difference between _insert and _replace is whether the old key or new > key gets freed if there is already an entry with that key. > > As the internal hash table used by RestParams doesn't free the key itself, this > is a moot point. > > (I suspect you are looking at an older codebase before RestParam landed)
Created attachment 279087 [details] [review] Use g_hash_table_replace() in rest_params_add() I came to the same conclusion as the reporter after hitting some memory corruption when using duplicate params: rest_params_add() is currently using g_hash_table_insert() to add the passed in parameter in the parameter hash table. The key which is used is owned by the associated value. When using rest_params_add to replace an already existing parameter, the existing value will be freed with rest_param_unref(). However, g_hash_table_insert() does not replace the key when it already exists in the hash table: "If the key already exists in the GHashTable its current value is replaced with the new value... If you supplied a key_destroy_func when creating the GHashTable, the passed key is freed using that function." This means that after replacing an already existing parameter, the corresponding key will still be the old one, which is now pointing at freed memory as the old value was freed. g_hash_table_replace() ensures that the key will still be valid, even when replacing existing parameters: "Inserts a new key and value into a GHashTable similar to g_hash_table_insert(). The difference is that if the key already exists in the GHashTable, it gets replaced by the new key."
*** Bug 731218 has been marked as a duplicate of this bug. ***
Review of attachment 279087 [details] [review]: not controversial to me, ack
Attachment 279087 [details] pushed as de21f49 - Use g_hash_table_replace() in rest_params_add()