On 12/10/2017 04:35 PM, Frediano Ziglio wrote:
Coverity complains node->flags is accessed without locking stat_file->lock.
However the function stat_file_add_node is only called (from
stat_file_add_counter) when the lock is taken.
No, this is false. The function handle the lock.
You are right. I made a mistake.
The problem reported is in stat_file_add_counter()
We can fix it by locking/unlocking there:
diff --git a/server/stat-file.c b/server/stat-file.c
index 2797fd739..84f409a46 100644
--- a/server/stat-file.c
+++ b/server/stat-file.c
@@ -189,8 +189,10 @@ stat_file_add_counter(RedStatFile *stat_file,
StatNodeRef parent, const char *na
if (ref == INVALID_STAT_REF) {
return NULL;
}
+ pthread_mutex_lock(&stat_file->lock);
node = &stat_file->stat->nodes[ref];
node->flags |= SPICE_STAT_NODE_FLAG_VALUE;
+ pthread_mutex_unlock(&stat_file->lock);
return &node->value;
}
Your solution below also looks good to me
and does not add another lock, but probably
better to pass "additional_flags" (or "extra_flags")
instead of "flags" such that visible ? ... would appear
once as it is currently in the code.
Thanks,
Uri.
The problem happens because stat_file_add_counter add a flag without locking.
However in this case this is not much of a problem as the this is the last
change of
the flag and maximum cause a temporary view of the node as not counter.
Add comment so it's clear.
Signed-off-by: Uri Lublin <uril@xxxxxxxxxx>
---
server/stat-file.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/server/stat-file.c b/server/stat-file.c
index 2797fd739..9aff8cd72 100644
--- a/server/stat-file.c
+++ b/server/stat-file.c
@@ -139,6 +139,7 @@ static void reds_insert_stat_node(RedStatFile
*stat_file,
StatNodeRef parent, St
}
}
+/* Called with stat_file->lock locked */
StatNodeRef
stat_file_add_node(RedStatFile *stat_file, StatNodeRef parent, const char
*name, int visible)
{
This should work
diff --git a/server/stat-file.c b/server/stat-file.c
index 2797fd73..c52c266c 100644
--- a/server/stat-file.c
+++ b/server/stat-file.c
@@ -139,8 +139,9 @@ static void reds_insert_stat_node(RedStatFile *stat_file, StatNodeRef parent, St
}
}
-StatNodeRef
-stat_file_add_node(RedStatFile *stat_file, StatNodeRef parent, const char *name, int visible)
+static StatNodeRef
+stat_file_add_node_flags(RedStatFile *stat_file, StatNodeRef parent, const char *name,
+ uint32_t flags)
{
StatNodeRef ref;
SpiceStatNode *node;
@@ -169,8 +170,7 @@ stat_file_add_node(RedStatFile *stat_file, StatNodeRef parent, const char *name,
stat_file->stat->generation++;
stat_file->stat->num_of_nodes++;
node->value = 0;
- node->flags = SPICE_STAT_NODE_FLAG_ENABLED |
- (visible ? SPICE_STAT_NODE_FLAG_VISIBLE : 0);
+ node->flags = SPICE_STAT_NODE_FLAG_ENABLED | flags;
g_strlcpy(node->name, name, sizeof(node->name));
reds_insert_stat_node(stat_file, parent, ref);
pthread_mutex_unlock(&stat_file->lock);
@@ -180,17 +180,25 @@ stat_file_add_node(RedStatFile *stat_file, StatNodeRef parent, const char *name,
return INVALID_STAT_REF;
}
+StatNodeRef
+stat_file_add_node(RedStatFile *stat_file, StatNodeRef parent, const char *name, int visible)
+{
+ uint32_t flags = visible ? SPICE_STAT_NODE_FLAG_VISIBLE : 0;
+ return stat_file_add_node_flags(stat_file, parent, name, flags);
+}
+
uint64_t *
stat_file_add_counter(RedStatFile *stat_file, StatNodeRef parent, const char *name, int visible)
{
- StatNodeRef ref = stat_file_add_node(stat_file, parent, name, visible);
+ uint32_t flags = visible ? SPICE_STAT_NODE_FLAG_VISIBLE : 0;
+ flags |= SPICE_STAT_NODE_FLAG_VALUE;
+ StatNodeRef ref = stat_file_add_node_flags(stat_file, parent, name, flags);
SpiceStatNode *node;
if (ref == INVALID_STAT_REF) {
return NULL;
}
node = &stat_file->stat->nodes[ref];
- node->flags |= SPICE_STAT_NODE_FLAG_VALUE;
return &node->value;
}
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel