Issue #49269: Fix coverity issues - 389-ds-base

389-ds-base

#49269 Fix coverity issues

Closed: wontfix 7 years ago Opened 7 years ago by mreynolds.

Issue Description

covscans reported a series of errors on master branch

Metadata Update from @mreynolds:
- Issue assigned to mreynolds

7 years ago

mreynolds commented 7 years ago

Aside from the fix above there are still some outstanding issues in src/libsds/sds/bpt/bpt.c

336#endif
   18. freed_arg: sds_bptree_branch_compact frees right. [show details]
337                    sds_bptree_branch_compact(binst, next_node, right);
338                    /* Setup the branch delete properly */
   CID 14992 (#2 of 2): Use after free (USE_AFTER_FREE)19. use_after_free: Using freed pointer right.
339                    next_node = right;
340                    deleted_node = right;
341                } else if (left != NULL && left->item_count < SDS_BPTREE_HALF_CAPACITY) {

and

345#endif
   34. freed_arg: sds_bptree_branch_compact frees next_node. [show details]
346                    sds_bptree_branch_compact(binst, left, next_node);
   CID 14993 (#2 of 2): Use after free (USE_AFTER_FREE)35. use_after_free: Using freed pointer next_node.
347                    deleted_node = next_node;
348                } else {
349                    /* Mate, if you get here you are fucked. */
350                    return SDS_INVALID_NODE;
351                }

To fix these I think we need to change sds_free(void) to sds_free (*void) so we can set the pointer to NULL after its freed. Like how we do things in slapi_ch_free(). I want to run this by you first @firstyear since this is your code.

And we should remove the profanity from the source code ;-)

mreynolds commented 7 years ago

Aside from the fix above there are still some outstanding issues in src/libsds/sds/bpt/bpt.c

336#endif
   18. freed_arg: sds_bptree_branch_compact frees right. [show details]
337                    sds_bptree_branch_compact(binst, next_node, right);
338                    /* Setup the branch delete properly */
   CID 14992 (#2 of 2): Use after free (USE_AFTER_FREE)19. use_after_free: Using freed pointer right.
339                    next_node = right;
340                    deleted_node = right;
341                } else if (left != NULL && left->item_count < SDS_BPTREE_HALF_CAPACITY) {

and

345#endif
   34. freed_arg: sds_bptree_branch_compact frees next_node. [show details]
346                    sds_bptree_branch_compact(binst, left, next_node);
   CID 14993 (#2 of 2): Use after free (USE_AFTER_FREE)35. use_after_free: Using freed pointer next_node.
347                    deleted_node = next_node;
348                } else {
349                    /* Mate, if you get here you are fucked. */
350                    return SDS_INVALID_NODE;
351                }

And we should remove the profanity from the source code ;-)

firstyear commented 7 years ago

In nunc-stans, line 1179:

178 -        internal_ns_job_rearm(job);
179          pthread_mutex_unlock(job->monitor);
180 +        internal_ns_job_rearm(job);

Please undo this. This is a recursive mutex, and it's important the lock is still help during the re-arm call, because it prevents a race condition that can cause the job not to be queued correctly when an external call occurs.

nunc-stans line 250,

140  
141      if (NS_JOB_IS_IO(job->job_type) || NS_JOB_IS_TIMER(job->job_type) || NS_JOB_IS_SIGNAL(job->job_type)) {
142          event_q_notify(job);
143 +        pthread_mutex_unlock(job->monitor);
144      } else {
145          /* if this is a non event task, just queue it on the work q */
146          /* Prevents an un-necessary queue / dequeue to the event_q */
147 +        pthread_mutex_unlock(job->monitor);
148          work_q_notify(job);
149      }
150 -    pthread_mutex_unlock(job->monitor);

The same is true here, You MUST unlock before you notify, because if the submitted job is in the state NS_JOB_NEEDS_DELETE, then the job can be freed before line 150 there is run, and you get a heap-use-after free. So please revert this also.

libsds in map nodes you change:

129 -                    tail->next = sds_malloc(sizeof(sds_bptree_node_list));
130 -                    tail = tail->next;
131 +                    tail = sds_malloc(sizeof(sds_bptree_node_list));

This changes the behaviour of map. We want tail->next && tail to be the same pointer here, so please undo this too;

120 -    sds_bptree_node_list *prev = cur;
121 +    sds_bptree_node_list *prev = NULL;

This too, we need prev == cur, because we do a pointer compare. Coverity complains about a use-after-free, but we don't re-use after the free, because we only use the pointer as a comparison.

Sorry to be such a pain, it's just that these parts of the code require quite a bit of time and analysis to make changes to :(

PS: The profanity improves the code, and speaks of my passion while coding ;). Anyway, if we want, we can remove it.

firstyear commented 7 years ago

Ohhh, and the rest looks good though :)

mreynolds commented 7 years ago

Thanks for the clarification! New patch attached:

firstyear commented 7 years ago

Thanks! Sorry to have been such a pain about the ns/sds code :(

Metadata Update from @firstyear:
- Custom field reviewstatus adjusted to ack (was: review)

7 years ago

mreynolds commented 7 years ago

Thanks! Sorry to have been such a pain about the ns/sds code :(

No , thank you for clarifying the issues, that's why I wanted you to look at it.

mreynolds commented 7 years ago

95a7f23..c2c512e master -> master

Metadata Update from @mreynolds:
- Issue close_status updated to: fixed
- Issue set to the milestone: 1.3.7.0
- Issue status updated to: Closed (was: Open)

7 years ago