[Spice-devel] [server PATCH 4/4] stat_file_add_node: add "locked" comment

[Frediano Ziglio]

> 
> 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 at redhat.com>
> >>> ---
> >>>   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
> > 
> 
>