[master] f8936e78a Change the panic reentrancy check so that Coverity can (hopefully) grok it.

[Dridi Boukelmoune]

On Mon, Dec 4, 2023 at 8:56 AM Poul-Henning Kamp <phk at freebsd.org> wrote:
>
>
> commit f8936e78a50153f3203bb077be8a067eac0c9663
> Author: Poul-Henning Kamp <phk at FreeBSD.org>
> Date:   Mon Dec 4 08:53:22 2023 +0000
>
>     Change the panic reentrancy check so that Coverity can (hopefully) grok it.

Defects eliminated: 2

That's a good start, but I think there were plenty more.

> diff --git a/bin/varnishd/cache/cache_main.c b/bin/varnishd/cache/cache_main.c
> index 3e093c50b..7b433c596 100644
> --- a/bin/varnishd/cache/cache_main.c
> +++ b/bin/varnishd/cache/cache_main.c
> @@ -81,6 +81,7 @@ static pthread_key_t req_key;
>  static pthread_key_t bo_key;
>  static pthread_key_t wrk_key;
>  pthread_key_t witness_key;
> +pthread_key_t panic_key;
>
>  void
>  THR_SetBusyobj(const struct busyobj *bo)
> @@ -404,6 +405,7 @@ child_main(int sigmagic, size_t altstksz)
>         PTOK(pthread_key_create(&wrk_key, NULL));
>         PTOK(pthread_key_create(&witness_key, free));
>         PTOK(pthread_key_create(&name_key, NULL));
> +       PTOK(pthread_key_create(&panic_key, NULL));
>
>         THR_SetName("cache-main");
>
> diff --git a/bin/varnishd/cache/cache_panic.c b/bin/varnishd/cache/cache_panic.c
> index 221d9835c..eca20c32b 100644
> --- a/bin/varnishd/cache/cache_panic.c
> +++ b/bin/varnishd/cache/cache_panic.c
> @@ -67,7 +67,6 @@
>
>  static struct vsb pan_vsb_storage, *pan_vsb;
>  static pthread_mutex_t panicstr_mtx;
> -static pthread_t panicy;
>
>  static void pan_sess(struct vsb *, const struct sess *);
>  static void pan_req(struct vsb *, const struct req *);
> @@ -744,18 +743,23 @@ pan_ic(const char *func, const char *file, int line, const char *cond,
>         struct busyobj *bo;
>         struct worker *wrk;
>         struct sigaction sa;
> -       int err = errno;
> +       int i, err = errno;
>
> -       /* If we already panicing in another thread, do nothing */
> -       while (heritage.panic_str[0] && panicy != pthread_self())
> -               sleep(1);
> -
> -       if (pthread_mutex_lock(&panicstr_mtx)) {
> -               /* Reentrant panic */
> +       if (pthread_getspecific(panic_key) != NULL) {
>                 VSB_cat(pan_vsb, "\n\nPANIC REENTRANCY\n\n");
>                 abort();
>         }
> -       panicy = pthread_self();
> +
> +       /* If we already panicing in another thread, do nothing */
> +       do {
> +               i = pthread_mutex_trylock(&panicstr_mtx);
> +               if (i != 0)
> +                       sleep (1);
> +       } while (i != 0);
> +
> +       assert (VSB_len(pan_vsb) == 0);
> +
> +       AZ(pthread_setspecific(panic_key, pan_vsb));
>
>         /*
>          * should we trigger a SIGSEGV while handling a panic, our sigsegv
> @@ -844,6 +848,14 @@ pan_ic(const char *func, const char *file, int line, const char *cond,
>         VSB_putc(pan_vsb, '\0');        /* NUL termination */
>
>         v_gcov_flush();
> +
> +       /*
> +        * Do a little song and dance for static checkers which
> +        * are not smart enough to figure out that calling abort()
> +        * with a mutex held is OK and probably very intentional.
> +        */
> +       if (pthread_getspecific(panic_key))     /* ie: always */
> +               abort();
>         PTOK(pthread_mutex_unlock(&panicstr_mtx));
>         abort();
>  }
> diff --git a/bin/varnishd/cache/cache_varnishd.h b/bin/varnishd/cache/cache_varnishd.h
> index b534eb174..e9ffad17c 100644
> --- a/bin/varnishd/cache/cache_varnishd.h
> +++ b/bin/varnishd/cache/cache_varnishd.h
> @@ -303,6 +303,7 @@ unsigned HTTP1_Write(const struct worker *w, const struct http *hp, const int*);
>
>  /* cache_main.c */
>  vxid_t VXID_Get(const struct worker *, uint64_t marker);
> +extern pthread_key_t panic_key;
>  extern pthread_key_t witness_key;
>
>  void THR_SetName(const char *name);
> _______________________________________________
> varnish-commit mailing list
> varnish-commit at varnish-cache.org
> https://www.varnish-cache.org/lists/mailman/listinfo/varnish-commit