https://fedorahosted.org/sssd/ticket/1799
One peculiarity of the sysdb_attrs_get_el interface is that if the attribute does not exist, then the attrs array is reallocated and the element is created. But in case other pointers are already pointing into the array, the realloc might invalidate them.
One such case was in the sdap_process_ghost_members function where if the group had no members, the "gh" pointer requested earlier might have been invalidated by the realloc in order to create the member element
I'm actually pondering the idea of renaming the interface to make it clear that if you request a nonexistent attr, the pointer already pointing there might become invalid.
On 04/12/2013 12:25 PM, Jakub Hrozek wrote:
https://fedorahosted.org/sssd/ticket/1799
One peculiarity of the sysdb_attrs_get_el interface is that if the attribute does not exist, then the attrs array is reallocated and the element is created. But in case other pointers are already pointing into the array, the realloc might invalidate them.
One such case was in the sdap_process_ghost_members function where if the group had no members, the "gh" pointer requested earlier might have been invalidated by the realloc in order to create the member element
I'm actually pondering the idea of renaming the interface to make it clear that if you request a nonexistent attr, the pointer already pointing there might become invalid.
Nack.
Please, add a comment explaining that you just create a dummy element that have zero values. Or set memberel->value and num_values explicitly.
Also, you don't really have to set memberel->name - it is not used anywhere and it doesn't add any semantic value. However, if you want to create a complete element, I'm ok with it.
Thanks.
On 04/18/2013 10:48 AM, Pavel Březina wrote:
On 04/12/2013 12:25 PM, Jakub Hrozek wrote:
https://fedorahosted.org/sssd/ticket/1799
One peculiarity of the sysdb_attrs_get_el interface is that if the attribute does not exist, then the attrs array is reallocated and the element is created. But in case other pointers are already pointing into the array, the realloc might invalidate them.
One such case was in the sdap_process_ghost_members function where if the group had no members, the "gh" pointer requested earlier might have been invalidated by the realloc in order to create the member element
I'm actually pondering the idea of renaming the interface to make it clear that if you request a nonexistent attr, the pointer already pointing there might become invalid.
Nack.
Please, add a comment explaining that you just create a dummy element that have zero values. Or set memberel->value and num_values explicitly.
Also, you don't really have to set memberel->name - it is not used anywhere and it doesn't add any semantic value. However, if you want to create a complete element, I'm ok with it.
Thanks.
Also the commit title is a little misleading. I thought that you changed one of the sysdb functions.
On Thu, Apr 18, 2013 at 10:50:55AM +0200, Pavel Březina wrote:
On 04/18/2013 10:48 AM, Pavel Březina wrote:
On 04/12/2013 12:25 PM, Jakub Hrozek wrote:
https://fedorahosted.org/sssd/ticket/1799
One peculiarity of the sysdb_attrs_get_el interface is that if the attribute does not exist, then the attrs array is reallocated and the element is created. But in case other pointers are already pointing into the array, the realloc might invalidate them.
One such case was in the sdap_process_ghost_members function where if the group had no members, the "gh" pointer requested earlier might have been invalidated by the realloc in order to create the member element
I'm actually pondering the idea of renaming the interface to make it clear that if you request a nonexistent attr, the pointer already pointing there might become invalid.
Nack.
Please, add a comment explaining that you just create a dummy element that have zero values. Or set memberel->value and num_values explicitly.
Also, you don't really have to set memberel->name - it is not used anywhere and it doesn't add any semantic value. However, if you want to create a complete element, I'm ok with it.
That was the intent, yes, but the function is so small we don't have to care, really. I added a comment and explicitly set memberel->value and num_values.
Thanks.
Also the commit title is a little misleading. I thought that you changed one of the sysdb functions.
Fixed the title.
On 04/18/2013 01:52 PM, Jakub Hrozek wrote:
On Thu, Apr 18, 2013 at 10:50:55AM +0200, Pavel Březina wrote:
On 04/18/2013 10:48 AM, Pavel Březina wrote:
On 04/12/2013 12:25 PM, Jakub Hrozek wrote:
https://fedorahosted.org/sssd/ticket/1799
One peculiarity of the sysdb_attrs_get_el interface is that if the attribute does not exist, then the attrs array is reallocated and the element is created. But in case other pointers are already pointing into the array, the realloc might invalidate them.
One such case was in the sdap_process_ghost_members function where if the group had no members, the "gh" pointer requested earlier might have been invalidated by the realloc in order to create the member element
I'm actually pondering the idea of renaming the interface to make it clear that if you request a nonexistent attr, the pointer already pointing there might become invalid.
Nack.
Please, add a comment explaining that you just create a dummy element that have zero values. Or set memberel->value and num_values explicitly.
Also, you don't really have to set memberel->name - it is not used anywhere and it doesn't add any semantic value. However, if you want to create a complete element, I'm ok with it.
That was the intent, yes, but the function is so small we don't have to care, really. I added a comment and explicitly set memberel->value and num_values.
Thanks.
Also the commit title is a little misleading. I thought that you changed one of the sysdb functions.
Fixed the title.
Ack.
On Fri, Apr 19, 2013 at 01:53:49PM +0200, Pavel Březina wrote:
On 04/18/2013 01:52 PM, Jakub Hrozek wrote:
On Thu, Apr 18, 2013 at 10:50:55AM +0200, Pavel Březina wrote:
On 04/18/2013 10:48 AM, Pavel Březina wrote:
On 04/12/2013 12:25 PM, Jakub Hrozek wrote:
https://fedorahosted.org/sssd/ticket/1799
One peculiarity of the sysdb_attrs_get_el interface is that if the attribute does not exist, then the attrs array is reallocated and the element is created. But in case other pointers are already pointing into the array, the realloc might invalidate them.
One such case was in the sdap_process_ghost_members function where if the group had no members, the "gh" pointer requested earlier might have been invalidated by the realloc in order to create the member element
I'm actually pondering the idea of renaming the interface to make it clear that if you request a nonexistent attr, the pointer already pointing there might become invalid.
Nack.
Please, add a comment explaining that you just create a dummy element that have zero values. Or set memberel->value and num_values explicitly.
Also, you don't really have to set memberel->name - it is not used anywhere and it doesn't add any semantic value. However, if you want to create a complete element, I'm ok with it.
That was the intent, yes, but the function is so small we don't have to care, really. I added a comment and explicitly set memberel->value and num_values.
Thanks.
Also the commit title is a little misleading. I thought that you changed one of the sysdb functions.
Fixed the title.
Ack.
Pushed to master and sssd-1-9
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Pavel Březina