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

[Victor Toso]

Hi,

On Mon, Jul 15, 2019 at 05:13:43AM -0400, Frediano Ziglio wrote:
> > 
> > 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.

Indeed, I guess I was a bit too focused on cleaning the list of
warnings and didn't pay that much attention. I'll rework this.

> Why you had to add an additional "pid" variable?

I usually cache the value in a local var instead of calling the
function more than once. Not a hard rule but I usually like the
code more.

Thanks,
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190715/ec377eb8/attachment.sig>