[sssd PR#447][opened] SYSDB: Remove code causing a covscan warning
by fidencio
URL: https://github.com/SSSD/sssd/pull/447
Author: fidencio
Title: #447: SYSDB: Remove code causing a covscan warning
Action: opened
PR body:
"""
There's no reason to check for both ret != EOK and sanitized == NULL, as
the second should never ever happen.
This check is causing a clang warning in our code:
Defect type: CLANG_WARNING
1. sssd-1.16.0/src/db/sysdb_ops.c:4223:9: warning: Dereference of undefined pointer value
# if (res->count > 1) {
# ^~~~~~~~~~
4. sssd-1.16.0/src/db/sysdb_ops.c:4199:5: note: 'res' declared without an initial value
# struct ldb_result *res;
# ^~~~~~~~~~~~~~~~~~~~~~
7. sssd-1.16.0/src/db/sysdb_ops.c:4202:9: note: Assuming 'sid_str' is non-null
# if (!sid_str) return EINVAL;
# ^~~~~~~~
10. sssd-1.16.0/src/db/sysdb_ops.c:4202:5: note: Taking false branch
# if (!sid_str) return EINVAL;
# ^
13. sssd-1.16.0/src/db/sysdb_ops.c:4205:9: note: Assuming 'tmp_ctx' is non-null
# if (!tmp_ctx) {
# ^~~~~~~~
16. sssd-1.16.0/src/db/sysdb_ops.c:4205:5: note: Taking false branch
# if (!tmp_ctx) {
# ^
19. sssd-1.16.0/src/db/sysdb_ops.c:4209:11: note: Calling 'sysdb_search_object_by_sid'
# ret = sysdb_search_object_by_sid(tmp_ctx, domain, sid_str, NULL, &res);
# ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
22. sssd-1.16.0/src/db/sysdb_ops.c:4960:12: note: Calling 'sysdb_search_object_by_str_attr'
# return sysdb_search_object_by_str_attr(mem_ctx, domain, SYSDB_SID_FILTER,
# ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
25. sssd-1.16.0/src/db/sysdb_ops.c:4872:5: note: Taking false branch
# if (str == NULL) {
# ^
28. sssd-1.16.0/src/db/sysdb_ops.c:4877:9: note: Assuming 'ret' is equal to 0
# if (ret != EOK || sanitized == NULL) {
# ^~~~~~~~~~
31. sssd-1.16.0/src/db/sysdb_ops.c:4877:9: note: Left side of '||' is false
32. sssd-1.16.0/src/db/sysdb_ops.c:4877:23: note: Assuming 'sanitized' is equal to null
# if (ret != EOK || sanitized == NULL) {
# ^~~~~~~~~~~~~~~~~
35. sssd-1.16.0/src/db/sysdb_ops.c:4877:5: note: Taking true branch
# if (ret != EOK || sanitized == NULL) {
# ^
38. sssd-1.16.0/src/db/sysdb_ops.c:4878:9: note: Left side of '||' is false
# DEBUG(SSSDBG_OP_FAILURE, "sss_filter_sanitize failed.\n");
# ^
41. sssd-1.16.0/src/util/debug.h:123:9: note: expanded from macro 'DEBUG'
# if (DEBUG_IS_SET(__debug_macro_level)) { \
# ^
44. sssd-1.16.0/src/util/debug.h:135:30: note: expanded from macro 'DEBUG_IS_SET'
# #define DEBUG_IS_SET(level) (debug_level & (level) || \
# ^
47. sssd-1.16.0/src/db/sysdb_ops.c:4878:9: note: Assuming 'debug_level' is not equal to 0
# DEBUG(SSSDBG_OP_FAILURE, "sss_filter_sanitize failed.\n");
# ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
50. sssd-1.16.0/src/util/debug.h:123:9: note: expanded from macro 'DEBUG'
# if (DEBUG_IS_SET(__debug_macro_level)) { \
# ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
53. sssd-1.16.0/src/util/debug.h:136:30: note: expanded from macro 'DEBUG_IS_SET'
# (debug_level == SSSDBG_UNRESOLVED && \
# ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
56. sssd-1.16.0/src/db/sysdb_ops.c:4878:9: note: Left side of '&&' is false
57. sssd-1.16.0/src/util/debug.h:123:9: note: expanded from macro 'DEBUG'
# if (DEBUG_IS_SET(__debug_macro_level)) { \
# ^
60. sssd-1.16.0/src/util/debug.h:136:63: note: expanded from macro 'DEBUG_IS_SET'
# (debug_level == SSSDBG_UNRESOLVED && \
# ^
63. sssd-1.16.0/src/db/sysdb_ops.c:4878:9: note: Loop condition is false. Exiting loop
64. sssd-1.16.0/src/util/debug.h:121:35: note: expanded from macro 'DEBUG'
# #define DEBUG(level, format, ...) do { \
# ^
67. sssd-1.16.0/src/db/sysdb_ops.c:4879:9: note: Control jumps to line 4892
# goto done;
# ^
70. sssd-1.16.0/src/db/sysdb_ops.c:4960:12: note: Returning from 'sysdb_search_object_by_str_attr'
# return sysdb_search_object_by_str_attr(mem_ctx, domain, SYSDB_SID_FILTER,
# ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
73. sssd-1.16.0/src/db/sysdb_ops.c:4209:11: note: Returning from 'sysdb_search_object_by_sid'
# ret = sysdb_search_object_by_sid(tmp_ctx, domain, sid_str, NULL, &res);
# ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
76. sssd-1.16.0/src/db/sysdb_ops.c:4211:5: note: Taking false branch
# if (ret == ENOENT) {
# ^
79. sssd-1.16.0/src/db/sysdb_ops.c:4217:12: note: Taking false branch
# } else if (ret != EOK) {
# ^
82. sssd-1.16.0/src/db/sysdb_ops.c:4223:9: note: Dereference of undefined pointer value
# if (res->count > 1) {
# ^~~~~~~~~~
# 4221| }
# 4222|
# 4223|-> if (res->count > 1) {
# 4224| DEBUG(SSSDBG_FATAL_FAILURE, "getbysid call returned more than one " \
# 4225| "result !?!\n");
Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com>
"""
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/447/head:pr447
git checkout pr447
6 years, 5 months
Question about memory mapped cache
by Sumit Bose
Hi,
while trying to prepare some diagrams to illustrate how the memory mapped
cache works I realized that we might not use the payload factor as
expected.
In sss_mmap_cache_init() we set a payload depending of the type of
cached data, e.g. for passwd data 4 * MC_SLOT_SIZE or for group data 3 *
MC_SLOT_SIZE. This value is used to calculate the data table size:
#define MC_DT_SIZE(elems, payload) ( (elems) * (payload) )
mc_ctx->dt_size = MC_DT_SIZE(n_elem, payload);
since this does not change the slot size there is now room for e.g
(n_elem * 4) slots for passwd data, which makes sense since we assume
that a typical passwd entry will occupy 4 slot and we want the cache to
be able to store n_elem passwd entries.
But the memory for the tree table is calculated only with n_elem
#define MC_FT_SIZE(elems) ( (elems) / 8 )
mc_ctx->ft_size = MC_FT_SIZE(n_elem);
This means that although we allocated memory for e.g. (4 * n_elem) slots
we can only handle n_elem slots because there are only n_elem bits in
the free table. As a result 3/4 of the allocate memory for the data is
unused and entries in the cache are overwritten earlier than needed.
Do you agree with this or did I miss something in the remaining part of
the code which makes sure the memory allocated for the data table can be
used completely?
If not I would suggest to make the payload only a factor which then can
be used to allocate the memory for the data table and the free table.
bye,
Sumit
6 years, 5 months