Fixed coverity in acl_hostch.cpp

[haoxian2]

Fixed coverity in acl_hostch.cpp: Dereference before null check (REVERSE_INULL)

Function was accessing pipe->host_pipe_info->m_lock before checking that pipe->host_pipe_info is NULL. If pipe->host_pipe_info is NULL, then it cannot be dereferenced.

[pcolberg]

Thanks @haoxian2! The change looks good, but the underlying issue leading to these mistakes is that the mutex is manually locked and unlocked. Would it be feasible to change host_pipe_t::m_lock from acl_mutex_t to std::mutex and use the RAII-safe std::scoped_lock to carry out the locking? (Admittedly one still has to get the locking after NULL check right since C++ does not provide a primitive to enforce locking, but cognitive overload is reduced significantly.)

[haoxian2]

Thanks @haoxian2! The change looks good, but the underlying issue leading to these mistakes is that the mutex is manually locked and unlocked. Would it be feasible to change host_pipe_t::m_lock from acl_mutex_t to std::mutex and use the RAII-safe std::scoped_lock to carry out the locking? (Admittedly one still has to get the locking after NULL check right since C++ does not provide a primitive to enforce locking, but cognitive overload is reduced significantly.)

@pcolberg I think it's feasible, a quick search indicates that host_pipe_t::m_lock are only present in these instances:

include/acl_types.h
723:  acl_mutex_t m_lock;

src/acl_hostch.cpp
256:  acl_mutex_lock(&(pipe->host_pipe_info->m_lock));
260:    acl_mutex_unlock(&(pipe->host_pipe_info->m_lock));
265:    acl_mutex_unlock(&(pipe->host_pipe_info->m_lock));
270:    acl_mutex_unlock(&(pipe->host_pipe_info->m_lock));
277:    acl_mutex_unlock(&(pipe->host_pipe_info->m_lock));
296:      acl_mutex_unlock(&(pipe->host_pipe_info->m_lock));
318:    acl_mutex_unlock(&(pipe->host_pipe_info->m_lock));
329:    acl_mutex_unlock(&(pipe->host_pipe_info->m_lock));
335:    acl_mutex_unlock(&(pipe->host_pipe_info->m_lock));
358:  acl_mutex_lock(&(pipe->host_pipe_info->m_lock));
361:    acl_mutex_unlock(&(pipe->host_pipe_info->m_lock));
366:    acl_mutex_unlock(&(pipe->host_pipe_info->m_lock));
371:    acl_mutex_unlock(&(pipe->host_pipe_info->m_lock));
384:    acl_mutex_unlock(&(pipe->host_pipe_info->m_lock));
410:    acl_mutex_unlock(&(pipe->host_pipe_info->m_lock));
431:      acl_mutex_unlock(&(pipe->host_pipe_info->m_lock));
449:  acl_mutex_unlock(&(pipe->host_pipe_info->m_lock));
473:  acl_mutex_lock(&(pipe->host_pipe_info->m_lock));
481:    acl_mutex_unlock(&(pipe->host_pipe_info->m_lock));
488:    acl_mutex_unlock(&(pipe->host_pipe_info->m_lock));
510:      acl_mutex_unlock(&(pipe->host_pipe_info->m_lock));
526:      acl_mutex_unlock(&(pipe->host_pipe_info->m_lock));
530:      acl_mutex_unlock(&(pipe->host_pipe_info->m_lock));
557:      acl_mutex_unlock(&(pipe->host_pipe_info->m_lock));
577:  acl_mutex_unlock(&(pipe->host_pipe_info->m_lock));
602:  acl_mutex_lock(&(pipe->host_pipe_info->m_lock));
606:    acl_mutex_unlock(&(pipe->host_pipe_info->m_lock));
627:    acl_mutex_unlock(&(pipe->host_pipe_info->m_lock));
635:    acl_mutex_unlock(&(pipe->host_pipe_info->m_lock));
680:  acl_mutex_unlock(&(pipe->host_pipe_info->m_lock));

src/acl_mem.cpp
233:        acl_mutex_destroy(&(mem->host_pipe_info->m_lock));
3976:    acl_mutex_init(&(host_pipe_info->m_lock), NULL);

Since it's mostly used in acl_mutex_lock and acl_mutex_unlock, I can look into changing those function to accept std::mutex instead of acl_mutex_t.

[pcolberg]

Since it's mostly used in acl_mutex_lock and acl_mutex_unlock, I can look into changing those function to accept std::mutex instead of acl_mutex_t.

Thanks @haoxian2 for looking into this. The replacement should be straight-forward, but since the number of occurrences you found is nontrivial it is better suited for separate PR.

An issue you may or may not run into is that the std::mutex constructor is not called, depending on how host_pipe_t objects are being created. This may be worked around using placement new, which invokes the constructor on the pointer to an already-allocated buffer, see e.g. 4febd02.

[pcolberg]

@zibaiwan @haoxian2 Keeping this PR unmerged for now since @sophimao is preparing a revision of #214 to address a performance regression on Windows, which can go into main for 2023.1 and save us a backport. Once completed, main can be branched into 2023.1, and this PR along with the remaining Coverity fixes can be merged for 2023.2.

[zibaiwan]

@pcolberg , I think it's safe to merge this PR now, just want to double confirm with you.