Insufficient locking in peas-engine.c
Hi, Red Hat static analysis has detected a bunch of race conditions in an old version of libpeas that are still valid in the current version. The results are not security-sensitive, so I will share them here. Looks like we just need to move more code under loaders_lock
. Since this is easy, I'll send a merge request.
"Error: LOCK_EVASION (CWE-543):
libpeas-1.30.0/libpeas/peas-engine.c:982: thread1_checks_field: Thread1 uses the value read from field ""enabled"" in the condition ""loader_info->enabled"". It sees that the condition is false. Control is switched to Thread2.
libpeas-1.30.0/libpeas/peas-engine.c:982: thread2_checks_field: Thread2 uses the value read from field ""enabled"" in the condition ""loader_info->enabled"". It sees that the condition is false.
libpeas-1.30.0/libpeas/peas-engine.c:985: thread2_acquires_lock: Thread2 acquires lock ""loaders_lock"".
libpeas-1.30.0/libpeas/peas-engine.c:992: thread2_modifies_field: Thread2 sets ""enabled"" to a new value. Note that this write can be reordered at runtime to occur before instructions that do not access this field within this locked region. After Thread2 leaves the critical section, control is switched back to Thread1.
libpeas-1.30.0/libpeas/peas-engine.c:985: thread1_acquires_lock: Thread1 acquires lock ""loaders_lock"".
libpeas-1.30.0/libpeas/peas-engine.c:992: thread1_overwrites_value_in_field: Thread1 sets ""enabled"" to a new value. Now the two threads have an inconsistent view of ""enabled"" and updates to fields correlated with ""enabled"" may be lost.
libpeas-1.30.0/libpeas/peas-engine.c:982: use_same_locks_for_read_and_modify: Guard the modification of ""enabled"" and the read used to decide whether to modify ""enabled"" with the same set of locks.
# 990| if (loaders[loader_id].enabled)
# 991| {
# 992|-> loader_info->enabled = TRUE;
# 993| g_mutex_unlock (&loaders_lock);
# 994| return;"
"Error: LOCK_EVASION (CWE-543):
libpeas-1.30.0/libpeas/peas-engine.c:982: thread1_checks_field: Thread1 uses the value read from field ""failed"" in the condition ""loader_info->failed"". It sees that the condition is false. Control is switched to Thread2.
libpeas-1.30.0/libpeas/peas-engine.c:982: thread2_checks_field: Thread2 uses the value read from field ""failed"" in the condition ""loader_info->failed"". It sees that the condition is false.
libpeas-1.30.0/libpeas/peas-engine.c:985: thread2_acquires_lock: Thread2 acquires lock ""loaders_lock"".
libpeas-1.30.0/libpeas/peas-engine.c:1017: thread2_modifies_field: Thread2 sets ""failed"" to a new value. Note that this write can be reordered at runtime to occur before instructions that do not access this field within this locked region. After Thread2 leaves the critical section, control is switched back to Thread1.
libpeas-1.30.0/libpeas/peas-engine.c:985: thread1_acquires_lock: Thread1 acquires lock ""loaders_lock"".
libpeas-1.30.0/libpeas/peas-engine.c:1017: thread1_overwrites_value_in_field: Thread1 sets ""failed"" to a new value. Now the two threads have an inconsistent view of ""failed"" and updates to fields correlated with ""failed"" may be lost.
libpeas-1.30.0/libpeas/peas-engine.c:982: use_same_locks_for_read_and_modify: Guard the modification of ""failed"" and the read used to decide whether to modify ""failed"" with the same set of locks.
# 1015| peas_utils_get_loader_from_id (loader_ids[i]));
# 1016|
# 1017|-> loader_info->failed = TRUE;
# 1018| loaders[loader_id].failed = TRUE;
# 1019| g_mutex_unlock (&loaders_lock);"
"Error: LOCK_EVASION (CWE-543):
libpeas-1.30.0/libpeas/peas-engine.c:904: thread1_checks_field: Thread1 uses the value read from field ""loader"" in the condition ""loader_info->loader != NULL"". It sees that the condition is false. Control is switched to Thread2.
libpeas-1.30.0/libpeas/peas-engine.c:904: thread2_checks_field: Thread2 uses the value read from field ""loader"" in the condition ""loader_info->loader != NULL"". It sees that the condition is false.
libpeas-1.30.0/libpeas/peas-engine.c:907: thread2_acquires_lock: Thread2 acquires lock ""loaders_lock"".
libpeas-1.30.0/libpeas/peas-engine.c:933: thread2_modifies_field: Thread2 sets ""loader"" to a new value. Note that this write can be reordered at runtime to occur before instructions that do not access this field within this locked region. After Thread2 leaves the critical section, control is switched back to Thread1.
libpeas-1.30.0/libpeas/peas-engine.c:907: thread1_acquires_lock: Thread1 acquires lock ""loaders_lock"".
libpeas-1.30.0/libpeas/peas-engine.c:933: thread1_overwrites_value_in_field: Thread1 sets ""loader"" to a new value. Now the two threads have an inconsistent view of ""loader"" and updates to fields of ""loader"" or fields correlated with ""loader"" may be lost.
libpeas-1.30.0/libpeas/peas-engine.c:904: use_same_locks_for_read_and_modify: Guard the modification of ""loader"" and the read used to decide whether to modify ""loader"" with the same set of locks.
# 931| }
# 932|
# 933|-> loader_info->loader = get_local_plugin_loader (engine, loader_id);
# 934|
# 935| if (loader_info->loader == NULL)"
"Error: LOCK_EVASION (CWE-543):
libpeas-1.30.0/libpeas/peas-engine.c:982: thread1_checks_field: Thread1 uses the value read from field ""enabled"" in the condition ""loader_info->enabled"". It sees that the condition is false. Control is switched to Thread2.
libpeas-1.30.0/libpeas/peas-engine.c:982: thread2_checks_field: Thread2 uses the value read from field ""enabled"" in the condition ""loader_info->enabled"". It sees that the condition is false.
libpeas-1.30.0/libpeas/peas-engine.c:985: thread2_acquires_lock: Thread2 acquires lock ""loaders_lock"".
libpeas-1.30.0/libpeas/peas-engine.c:1027: thread2_modifies_field: Thread2 sets ""enabled"" to a new value. Note that this write can be reordered at runtime to occur before instructions that do not access this field within this locked region. After Thread2 leaves the critical section, control is switched back to Thread1.
libpeas-1.30.0/libpeas/peas-engine.c:985: thread1_acquires_lock: Thread1 acquires lock ""loaders_lock"".
libpeas-1.30.0/libpeas/peas-engine.c:1027: thread1_overwrites_value_in_field: Thread1 sets ""enabled"" to a new value. Now the two threads have an inconsistent view of ""enabled"" and updates to fields correlated with ""enabled"" may be lost.
libpeas-1.30.0/libpeas/peas-engine.c:982: use_same_locks_for_read_and_modify: Guard the modification of ""enabled"" and the read used to decide whether to modify ""enabled"" with the same set of locks.
# 1025| * load it in get_plugin_loader() so that it is loaded lazily.
# 1026| */
# 1027|-> loader_info->enabled = TRUE;
# 1028| loaders[loader_id].enabled = TRUE;
# 1029|
"
"Error: LOCK_EVASION (CWE-543):
libpeas-1.30.0/libpeas/peas-engine.c:904: thread1_checks_field: Thread1 uses the value read from field ""failed"" in the condition ""loader_info->failed"". It sees that the condition is false. Control is switched to Thread2.
libpeas-1.30.0/libpeas/peas-engine.c:904: thread2_checks_field: Thread2 uses the value read from field ""failed"" in the condition ""loader_info->failed"". It sees that the condition is false.
libpeas-1.30.0/libpeas/peas-engine.c:907: thread2_acquires_lock: Thread2 acquires lock ""loaders_lock"".
libpeas-1.30.0/libpeas/peas-engine.c:936: thread2_modifies_field: Thread2 sets ""failed"" to a new value. Note that this write can be reordered at runtime to occur before instructions that do not access this field within this locked region. After Thread2 leaves the critical section, control is switched back to Thread1.
libpeas-1.30.0/libpeas/peas-engine.c:907: thread1_acquires_lock: Thread1 acquires lock ""loaders_lock"".
libpeas-1.30.0/libpeas/peas-engine.c:936: thread1_overwrites_value_in_field: Thread1 sets ""failed"" to a new value. Now the two threads have an inconsistent view of ""failed"" and updates to fields correlated with ""failed"" may be lost.
libpeas-1.30.0/libpeas/peas-engine.c:904: use_same_locks_for_read_and_modify: Guard the modification of ""failed"" and the read used to decide whether to modify ""failed"" with the same set of locks.
# 934|
# 935| if (loader_info->loader == NULL)
# 936|-> loader_info->failed = TRUE;
# 937|
# 938| g_mutex_unlock (&loaders_lock);"