Since 1.3.7.0 the pblock struct is a split in sub-structs (https://pagure.io/389-ds-base/issue/49097)
Before, it was a quite flat calloc struct and any slapi-pblock-get retrieved the field (NULL if not previously slapi_pblock_set) and assigned the provided variable.
Now, the sub-struct are allocated on demand (slapi_pblock_set). If a substruct that contains the requested field is not allocated the provided variable is unchanged.
This is a change of behavior, because a uninitialized local variable can get random value (stack) if the lookup field/struct has not been set.
Applies to 1.3.7.0 and after
It does not apply in 1.3.6 because 49097 was not fixed the same way in the two branches
Crash
#0 0x00007f411e5ee939 in __strcasecmp_l_sse2 #1 0x00007f41151e5761 in ipa_topo_check_segment_updates #2 0x00007f41151e5c78 in ipa_topo_pre_mod #3 0x00007f41216c1f74 in plugin_call_func #4 0x00007f41216c2203 in plugin_call_list #5 0x00007f41216c2203 in plugin_call_plugins #6 0x00007f412167c00a in dse_modify #7 0x00007f41216ae456 in op_shared_modify #8 0x00007f41216af8eb in do_modify #9 0x0000556e159e32b7 in connection_dispatch_operation #10 0x0000556e159e32b7 in connection_threadmain #11 0x00007f411f60dc1b in _pt_root #12 0x00007f411efade25 in start_thread #13 0x00007f411e65c1ad in clone
Should not crash
Metadata Update from @tbordaz: - Issue assigned to tbordaz
<img alt="0001-Ticket-49394-slapi_pblock_get-may-leave-unchanged-th.patch" src="/389-ds-base/issue/raw/files/52a730334c9a43e50db48cfe43ddfbf9de10dba9a52d37485bc416be10868b4c-0001-Ticket-49394-slapi_pblock_get-may-leave-unchanged-th.patch" />
Metadata Update from @tbordaz: - Custom field component adjusted to None - Custom field origin adjusted to None - Custom field reviewstatus adjusted to None - Custom field type adjusted to None - Custom field version adjusted to None
Metadata Update from @mreynolds: - Custom field reviewstatus adjusted to ack (was: None)
Thanks Mark.. but I put the patch just to not forget it.. I have not yet tested (even compile) it ;)
Un-acked ;-)
Metadata Update from @mreynolds: - Custom field reviewstatus adjusted to None (was: ack)
why do you need to handle it in every case, can't you just set a default at the start ?
slapi_pblock_get(Slapi_PBlock *pblock, int arg, void *value) ... if (value) *(void *)value = NULL; ....
Yes this is what I thought initially but I had a doubt.
If 'value' is actually a pointer to a 32bits variable, I wonder if '(void )value=NULL' would not overflow the 32bits and break an other variable on the caller stack.
This is indeed a valid concern.
Metadata Update from @mreynolds: - Issue set to the milestone: 1.3.6.0
Really isn't the problem here that the caller is assuming we'll sanitise the data for them? Why not just fix the callers to set to 0 by default?
Another issue is that there is no difference between an "unset" value and a "0 value", which could cause other issues. But I guess we are restoring the initial behaviour, and the pblock_get interface will go away soon enough anyway.
Also, this shows up really easily in ASAN because all the values on the heap/stack start with a poison value of like 0xfefefefefefefefe, so it's really obvious when it's not set correctly btw. So this behaviour likely has not affected core DS code, only IPA plugins ....
There is a difference between an unset and "0" if the caller test the value to be NULL. Before, pblock was calloc and returned value was "0" if the field was not previously set.
I do agree it good to initialize local variable but some plugins (being lazy) have worked so far. now the result is random and we can not require all users to sanitise/recompile there plugins.
<img alt="0001-Ticket-49394-slapi_pblock_get-may-leave-unchanged-th.patch" src="/389-ds-base/issue/raw/files/01f10c82027d2c59c6be5c70bb6795449a976069a92db9f0fd88dc08fad376c0-0001-Ticket-49394-slapi_pblock_get-may-leave-unchanged-th.patch" />
Metadata Update from @tbordaz: - Custom field reviewstatus adjusted to review (was: None)
Metadata Update from @firstyear: - Custom field reviewstatus adjusted to ack (was: review)
git push origin master
Counting objects: 6, done. Delta compression using up to 8 threads. Compressing objects: 100% (6/6), done. Writing objects: 100% (6/6), 2.20 KiB | 0 bytes/s, done. Total 6 (delta 4), reused 0 (delta 0) To ssh://git@pagure.io/389-ds-base.git 6b6878b..dcba969 master -> master
git push origin master (build warning)
Counting objects: 6, done. Delta compression using up to 8 threads. Compressing objects: 100% (6/6), done. Writing objects: 100% (6/6), 613 bytes | 0 bytes/s, done. Total 6 (delta 4), reused 0 (delta 0) To ssh://git@pagure.io/389-ds-base.git 1e28c75..34a184a master -> master
git push origin 389-ds-base-1.3.7
Counting objects: 6, done. Delta compression using up to 8 threads. Compressing objects: 100% (6/6), done. Writing objects: 100% (6/6), 2.21 KiB | 0 bytes/s, done. Total 6 (delta 4), reused 0 (delta 0) To ssh://git@pagure.io/389-ds-base.git 4471b73..746abe7 389-ds-base-1.3.7 -> 389-ds-base-1.3.7
This bug does not exist in 389-ds-base-1.3.6
https://pagure.io/389-ds-base/issue/49097 was pushed into two main pushes:
In 1.3.6, there was only the first push. So even if #49097 is in the log of 1.3.6, #49394 does not exist in that version.
Closing the ticket
Metadata Update from @tbordaz: - Issue close_status updated to: fixed - Issue set to the milestone: 1.3.7.0 (was: 1.3.6.0) - Issue status updated to: Closed (was: Open)
389-ds-base is moving from Pagure to Github. This means that new issues and pull requests will be accepted only in 389-ds-base's github repository.
This issue has been cloned to Github and is available here: - https://github.com/389ds/389-ds-base/issues/2453
If you want to receive further updates on the issue, please navigate to the github issue and click on subscribe button.
subscribe
Thank you for understanding. We apologize for all inconvenience.
Metadata Update from @spichugi: - Issue close_status updated to: wontfix (was: fixed)
Log in to comment on this ticket.