[PATCH] scripts/coccinelle: Catch dubious code after &error_abort/&error_fatal

[PATCH] scripts/coccinelle: Catch dubious code after &error_abort/&error_fatal

[Philippe Mathieu-Daudé]

Calls passing &error_abort or &error_fatal don't return.
Any code after such use is dubious. Add a coccinelle patch
to detect such pattern.

Inspired-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 .../use-after-abort-fatal-errp.cocci          | 33 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 34 insertions(+)
 create mode 100644 scripts/coccinelle/use-after-abort-fatal-errp.cocci

diff --git a/scripts/coccinelle/use-after-abort-fatal-errp.cocci b/scripts/coccinelle/use-after-abort-fatal-errp.cocci
new file mode 100644
index 00000000000..ead9de5826a
--- /dev/null
+++ b/scripts/coccinelle/use-after-abort-fatal-errp.cocci
@@ -0,0 +1,33 @@
+/* Find dubious code use after error_abort/error_fatal
+ *
+ * Inspired by this patch:
+ * https://www.mail-archive.com/qemu-devel@nongnu.org/msg789501.html
+ *
+ * Copyright (C) 2121 Red Hat, Inc.
+ *
+ * Authors:
+ *  Philippe Mathieu-Daudé
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+@@
+identifier func_with_errp;
+@@
+(
+ if (func_with_errp(..., &error_fatal)) {
+    /* Used for displaying help message */
+    ...
+    exit(...);
+  }
+|
+*if (func_with_errp(..., &error_fatal)) {
+    /* dubious code */
+    ...
+  }
+|
+*if (func_with_errp(..., &error_abort)) {
+    /* dubious code */
+    ...
+  }
+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 1e15dab8cd4..db6596eb06d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2368,6 +2368,7 @@ F: scripts/coccinelle/error_propagate_null.cocci
 F: scripts/coccinelle/remove_local_err.cocci
 F: scripts/coccinelle/use-error_fatal.cocci
 F: scripts/coccinelle/errp-guard.cocci
+F: scripts/coccinelle/use-after-abort-fatal-errp.cocci
 
 GDB stub
 M: Alex Bennée <alex.bennee@linaro.org>
-- 
2.26.2

Re: [PATCH] scripts/coccinelle: Catch dubious code after &error_abort/&error_fatal

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> Calls passing &error_abort or &error_fatal don't return.

Correction: they *can* return.  What they can't is return failure.

> Any code after such use is dubious. Add a coccinelle patch
> to detect such pattern.

To be precise: any failure path from such a call is dead code.

> Inspired-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  .../use-after-abort-fatal-errp.cocci          | 33 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 34 insertions(+)
>  create mode 100644 scripts/coccinelle/use-after-abort-fatal-errp.cocci
>
> diff --git a/scripts/coccinelle/use-after-abort-fatal-errp.cocci b/scripts/coccinelle/use-after-abort-fatal-errp.cocci
> new file mode 100644
> index 00000000000..ead9de5826a
> --- /dev/null
> +++ b/scripts/coccinelle/use-after-abort-fatal-errp.cocci
> @@ -0,0 +1,33 @@
> +/* Find dubious code use after error_abort/error_fatal
> + *
> + * Inspired by this patch:
> + * https://www.mail-archive.com/qemu-devel@nongnu.org/msg789501.html
> + *
> + * Copyright (C) 2121 Red Hat, Inc.
> + *
> + * Authors:
> + *  Philippe Mathieu-Daudé
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +@@
> +identifier func_with_errp;
> +@@
> +(
> + if (func_with_errp(..., &error_fatal)) {
> +    /* Used for displaying help message */
> +    ...
> +    exit(...);
> +  }
> +|
> +*if (func_with_errp(..., &error_fatal)) {
> +    /* dubious code */
> +    ...
> +  }
> +|
> +*if (func_with_errp(..., &error_abort)) {
> +    /* dubious code */
> +    ...
> +  }
> +)

This assumes func_with_errp() returns a falsy value on failure.
Functions returning bool and pointers do that by convention, but not
functions returning (signed) integers:

 * - Whenever practical, also return a value that indicates success /
 *   failure.  This can make the error checking more concise, and can
 *   avoid useless error object creation and destruction.  Note that
 *   we still have many functions returning void.  We recommend
 *   • bool-valued functions return true on success / false on failure,
 *   • pointer-valued functions return non-null / null pointer, and
 *   • integer-valued functions return non-negative / negative.

Special case of integer-valued functions: return zero / negative.  Your
script gets that one backwards, I'm afraid.

Moreover, I expect the convention to be violated in places.

I'm converned about the script's rate of false positives.  How many
reports do you get for the whole tree?  Can you guesstimate the rate of
false positives?

Next issue.  We commonly assign the return value to a variable before
checking it, like this:

    ret = func_with_errp(..., errp);
    if (!ret) {
        ...
        return some error value;
    }
    do something with @ret...

I suspect your script can't flag dead error handling there.  False
negatives.  These merely make the script less useful, whereas false
positives make it less usable, which is arguably worse.

> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1e15dab8cd4..db6596eb06d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2368,6 +2368,7 @@ F: scripts/coccinelle/error_propagate_null.cocci
>  F: scripts/coccinelle/remove_local_err.cocci
>  F: scripts/coccinelle/use-error_fatal.cocci
>  F: scripts/coccinelle/errp-guard.cocci
> +F: scripts/coccinelle/use-after-abort-fatal-errp.cocci
>  
>  GDB stub
>  M: Alex Bennée <alex.bennee@linaro.org>

Re: [PATCH] scripts/coccinelle: Catch dubious code after &error_abort/&error_fatal

[Philippe Mathieu-Daudé]

On 3/12/21 9:58 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
>> Calls passing &error_abort or &error_fatal don't return.
> 
> Correction: they *can* return.  What they can't is return failure.
> 
>> Any code after such use is dubious. Add a coccinelle patch
>> to detect such pattern.
> 
> To be precise: any failure path from such a call is dead code.
> 
>> Inspired-by: Kevin Wolf <kwolf@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  .../use-after-abort-fatal-errp.cocci          | 33 +++++++++++++++++++
>>  MAINTAINERS                                   |  1 +
>>  2 files changed, 34 insertions(+)
>>  create mode 100644 scripts/coccinelle/use-after-abort-fatal-errp.cocci
>>
>> diff --git a/scripts/coccinelle/use-after-abort-fatal-errp.cocci b/scripts/coccinelle/use-after-abort-fatal-errp.cocci
>> new file mode 100644
>> index 00000000000..ead9de5826a
>> --- /dev/null
>> +++ b/scripts/coccinelle/use-after-abort-fatal-errp.cocci
>> @@ -0,0 +1,33 @@
>> +/* Find dubious code use after error_abort/error_fatal
>> + *
>> + * Inspired by this patch:
>> + * https://www.mail-archive.com/qemu-devel@nongnu.org/msg789501.html
>> + *
>> + * Copyright (C) 2121 Red Hat, Inc.
>> + *
>> + * Authors:
>> + *  Philippe Mathieu-Daudé
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +@@
>> +identifier func_with_errp;
>> +@@
>> +(
>> + if (func_with_errp(..., &error_fatal)) {
>> +    /* Used for displaying help message */
>> +    ...
>> +    exit(...);
>> +  }
>> +|
>> +*if (func_with_errp(..., &error_fatal)) {
>> +    /* dubious code */
>> +    ...
>> +  }
>> +|
>> +*if (func_with_errp(..., &error_abort)) {
>> +    /* dubious code */
>> +    ...
>> +  }
>> +)
> 
> This assumes func_with_errp() returns a falsy value on failure.
> Functions returning bool and pointers do that by convention, but not
> functions returning (signed) integers:
> 
>  * - Whenever practical, also return a value that indicates success /
>  *   failure.  This can make the error checking more concise, and can
>  *   avoid useless error object creation and destruction.  Note that
>  *   we still have many functions returning void.  We recommend
>  *   • bool-valued functions return true on success / false on failure,
>  *   • pointer-valued functions return non-null / null pointer, and
>  *   • integer-valued functions return non-negative / negative.
> 
> Special case of integer-valued functions: return zero / negative.  Your
> script gets that one backwards, I'm afraid.

Apparently:

hw/ppc/spapr.c
---
@@ -2900,7 +2900,6 @@ static void spapr_machine_init(MachineSt
     }

     /* Graphics */
*    if (spapr_vga_init(phb->bus, &error_fatal)) {
         spapr->has_graphics = true;
         machine->usb |= defaults_enabled() && !machine->usb_disabled;
     }
---

qemu-img.c
---
@@ -589,9 +589,6 @@ static int img_create(int argc, char **a
     }
     optind++;

*    if (qemu_opts_foreach(&qemu_object_opts,
*                          user_creatable_add_opts_foreach,
*                          qemu_img_object_print_help, &error_fatal)) {
         goto fail;
     }
---

> Moreover, I expect the convention to be violated in places.
> 
> I'm converned about the script's rate of false positives.  How many
> reports do you get for the whole tree?  Can you guesstimate the rate of
> false positives?
> 
> Next issue.  We commonly assign the return value to a variable before
> checking it, like this:
> 
>     ret = func_with_errp(..., errp);
>     if (!ret) {
>         ...
>         return some error value;
>     }
>     do something with @ret...

Indeed I couldn't catch that easily.

> 
> I suspect your script can't flag dead error handling there.  False
> negatives.  These merely make the script less useful, whereas false
> positives make it less usable, which is arguably worse.

OK, thanks for your analysis, let's drop this patch.