>
> 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;
> }
>
Why not
return NULL;
}
node = &stat_file->stat->nodes[ref];
- node->flags |= SPICE_STAT_NODE_FLAG_VALUE;
+ __sync_or_and_fetch(&node->flags, SPICE_STAT_NODE_FLAG_VALUE);
return &node->value;
}
static void stat_file_remove(RedStatFile *stat_file, SpiceStatNode *node)
we already use __sync builtins in utils.h.
>
> 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