| Message ID | 20211208084022.95551-1-james.hilliard1@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | notifier: make access to notifiers thread-safe | expand |
James Hilliard wrote on Wed, Dec 08, 2021 at 01:40:22AM -0700: > From: saproj <saproj@gmail.com> Might want to put his real name here? (looks like 'Sergei Antonov') > Race condition happened between register_notifier(), which adds > notifiers to the list, and notify(), which iterates over the list. > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> Thanks! I had noticed a similar race between register_notifier() itself in two different threads with helgrind, but didn't pursue it further as it wasn't the problem I was looking at -- I think it's good to clear this up. Reviewed-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
Hi James, thanks for picking it up or it was lost: On 08.12.21 09:49, Dominique MARTINET wrote: > James Hilliard wrote on Wed, Dec 08, 2021 at 01:40:22AM -0700: >> From: saproj <saproj@gmail.com> > > Might want to put his real name here? > (looks like 'Sergei Antonov') Sergei, Is it correct ? No need to repost, I will do myself by merging adding your signed-off: Signed-off-by: Sergei Antonov <aproj@gmail.com> Is it ok ? > >> Race condition happened between register_notifier(), which adds >> notifiers to the list, and notify(), which iterates over the list. >> >> Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > > Thanks! I had noticed a similar race between register_notifier() itself > in two different threads with helgrind, but didn't pursue it further as > it wasn't the problem I was looking at -- I think it's good to clear > this up. > > Reviewed-by: Dominique Martinet <dominique.martinet@atmark-techno.com> > Best regards, Stefano Babic
On 08.12.21 11:23, Stefano Babic wrote: > Hi James, > > thanks for picking it up or it was lost: > > On 08.12.21 09:49, Dominique MARTINET wrote: >> James Hilliard wrote on Wed, Dec 08, 2021 at 01:40:22AM -0700: >>> From: saproj <saproj@gmail.com> >> >> Might want to put his real name here? >> (looks like 'Sergei Antonov') > > Sergei, Is it correct ? No need to repost, I will do myself by merging > adding your signed-off: > > Signed-off-by: Sergei Antonov <aproj@gmail.com> > > Is it ok ? > Sorry for noise, Signed-off-by was already correct. Applied to -master, thanks ! Best regards, Stefano Babic >> >>> Race condition happened between register_notifier(), which adds >>> notifiers to the list, and notify(), which iterates over the list. >>> >>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com> >> >> Thanks! I had noticed a similar race between register_notifier() itself >> in two different threads with helgrind, but didn't pursue it further as >> it wasn't the problem I was looking at -- I think it's good to clear >> this up. >> >> Reviewed-by: Dominique Martinet <dominique.martinet@atmark-techno.com> >> > > Best regards, > Stefano Babic >
diff --git a/core/notifier.c b/core/notifier.c index 2d471d9..77cab01 100644 --- a/core/notifier.c +++ b/core/notifier.c @@ -40,6 +40,7 @@ struct notify_elem { STAILQ_HEAD(notifylist, notify_elem); static struct notifylist clients; +static pthread_mutex_t clients_mutex; /* * Notification can be sent even by other @@ -203,7 +204,9 @@ int register_notifier(notifier client) return -ENOMEM; newclient->client = client; + pthread_mutex_lock(&clients_mutex); STAILQ_INSERT_TAIL(&clients, newclient, next); + pthread_mutex_unlock(&clients_mutex); return 0; } @@ -235,8 +238,10 @@ void notify(RECOVERY_STATUS status, int error, int level, const char *msg) sizeof(struct sockaddr_un)); } } else { /* Main process */ + pthread_mutex_lock(&clients_mutex); STAILQ_FOREACH(elem, &clients, next) (elem->client)(status, error, level, msg); + pthread_mutex_unlock(&clients_mutex); } } @@ -524,6 +529,7 @@ void notify_init(void) */ addr_init(¬ify_server, "NotifyServer"); STAILQ_INIT(&clients); + pthread_mutex_init(&clients_mutex, NULL); register_notifier(console_notifier); register_notifier(process_notifier); register_notifier(progress_notifier);