[Spice-devel] [linux/vd-agent v1 3/7] covscan: check return value of fprintf

[Frediano Ziglio]

> 
> From: Victor Toso <me at victortoso.com>
> 
>  | Error: CHECKED_RETURN (CWE-252):
>  | spice-vdagent-0.19.0/src/vdagentd/vdagentd.c:999: check_return: Calling
>  | "fprintf" without checking return value (as is done elsewhere 29 out of
>  | 30 times).
>  | spice-vdagent-0.19.0/src/vdagentd/xorg-conf.c:95: example_assign: Example
>  | 1: Assigning: "r" = return value from "fprintf(f, "# xorg.conf generated
>  | by spice-vdagentd\n")".
>  | spice-vdagent-0.19.0/src/vdagentd/xorg-conf.c:95: example_checked: Example
>  | 1 (cont.): "r" has its value checked in "r < 0".
>  | spice-vdagent-0.19.0/src/vdagentd/xorg-conf.c:96: example_assign: Example
>  | 2: Assigning: "r" = return value from "fprintf(f, "# generated from
>  | monitor info provided by the client\n\n")".
>  | spice-vdagent-0.19.0/src/vdagentd/xorg-conf.c:96: example_checked: Example
>  | 2 (cont.): "r" has its value checked in "r < 0".
>  | spice-vdagent-0.19.0/src/vdagentd/xorg-conf.c:99: example_assign: Example
>  | 3: Assigning: "r" = return value from "fprintf(f, "# Client has only 1
>  | monitor\n")".
>  | spice-vdagent-0.19.0/src/vdagentd/xorg-conf.c:99: example_checked: Example
>  | 3 (cont.): "r" has its value checked in "r < 0".
>  | spice-vdagent-0.19.0/src/vdagentd/xorg-conf.c:100: example_assign: Example
>  | 4: Assigning: "r" = return value from "fprintf(f, "# This works best with
>  | no xorg.conf, leaving xorg.conf empty\n")".
>  | spice-vdagent-0.19.0/src/vdagentd/xorg-conf.c:100: example_checked:
>  | Example 4 (cont.): "r" has its value checked in "r < 0".
>  | spice-vdagent-0.19.0/src/vdagentd/xorg-conf.c:106: example_assign: Example
>  | 5: Assigning: "r" = return value from "fprintf(f, "Section
>  | \"ServerFlags\"\n")".
>  | spice-vdagent-0.19.0/src/vdagentd/xorg-conf.c:106: example_checked:
>  | Example 5 (cont.): "r" has its value checked in "r < 0".
>  | #  997|           pidfile = fopen(pidfilename, "w");
>  | #  998|           if (pidfile) {
>  | #  999|->             fprintf(pidfile, "%d\n", (int)getpid());
>  | # 1000|               fclose(pidfile);
>  | # 1001|           }
> 
> Signed-off-by: Victor Toso <victortoso at redhat.com>
> ---
>  src/vdagentd/vdagentd.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index 72a3e13..63f3a12 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -996,7 +996,11 @@ static void daemonize(void)
>          }
>          pidfile = fopen(pidfilename, "w");
>          if (pidfile) {
> -            fprintf(pidfile, "%d\n", (int)getpid());
> +            int pid = (int) getpid();
> +            int r = fprintf(pidfile, "%d\n", pid);
> +            if (r < 0) {
> +                syslog(LOG_ERR, "Failure to write pid %d to file %s", pid,
> pidfilename);
> +            }
>              fclose(pidfile);
>          }
>          break;

I would say it's pretty weird to give error if a rare situation happens
(unable to write small data) but if we cannot create the file (which
is much more common) there's no warning/error.
Why you had to add an additional "pid" variable?

Frediano