| Message ID | 20210729110647.25500-1-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools/xenstored: Don't assume errno will not be overwritten in lu_arch() | expand |
On 29/07/2021 12:06, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > At the moment, do_control_lu() will set errno to 0 before calling > lu_arch() and then check errno. The expectation is nothing in lu_arch() > will change the value unless there is an error. > > However, per errno(3), a function that succeeds is allowed to change > errno. In fact, syslog() will overwrite errno if the logs are rotated > at the time it is called. > > To prevent any further issue, errno is now always set before > returning NULL. > > Additionally, errno is only checked when returning NULL so the client > can see the error message if there is any. > > Reported-by: Michael Kurth <mku@amazon.com> > Signed-off-by: Julien Grall <jgrall@amazon.com> > --- > tools/xenstore/xenstored_control.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/tools/xenstore/xenstored_control.c b/tools/xenstore/xenstored_control.c > index 6b68b79faac7..6fcb42095b59 100644 > --- a/tools/xenstore/xenstored_control.c > +++ b/tools/xenstore/xenstored_control.c > @@ -324,6 +324,7 @@ static const char *lu_binary_alloc(const void *ctx, struct connection *conn, > lu_status->kernel_size = size; > lu_status->kernel_off = 0; > > + errno = 0; > return NULL; > } > > @@ -339,6 +340,7 @@ static const char *lu_binary_save(const void *ctx, struct connection *conn, > memcpy(lu_status->kernel + lu_status->kernel_off, data, size); > lu_status->kernel_off += size; > > + errno = 0; > return NULL; > } > I forgot to update lu_binary(). I will respin the patch once I get some feedback. Cheers, > @@ -798,9 +800,8 @@ static int do_control_lu(void *ctx, struct connection *conn, > if (!ret) > return errno; > } else { > - errno = 0; > ret = lu_arch(ctx, conn, vec, num); > - if (errno) > + if (!ret && errno) > return errno; > } > >
On 29.07.21 17:23, Julien Grall wrote: > > > On 29/07/2021 12:06, Julien Grall wrote: >> From: Julien Grall <jgrall@amazon.com> >> >> At the moment, do_control_lu() will set errno to 0 before calling >> lu_arch() and then check errno. The expectation is nothing in lu_arch() >> will change the value unless there is an error. >> >> However, per errno(3), a function that succeeds is allowed to change >> errno. In fact, syslog() will overwrite errno if the logs are rotated >> at the time it is called. >> >> To prevent any further issue, errno is now always set before >> returning NULL. >> >> Additionally, errno is only checked when returning NULL so the client >> can see the error message if there is any. >> >> Reported-by: Michael Kurth <mku@amazon.com> >> Signed-off-by: Julien Grall <jgrall@amazon.com> >> --- >> tools/xenstore/xenstored_control.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/tools/xenstore/xenstored_control.c >> b/tools/xenstore/xenstored_control.c >> index 6b68b79faac7..6fcb42095b59 100644 >> --- a/tools/xenstore/xenstored_control.c >> +++ b/tools/xenstore/xenstored_control.c >> @@ -324,6 +324,7 @@ static const char *lu_binary_alloc(const void >> *ctx, struct connection *conn, >> lu_status->kernel_size = size; >> lu_status->kernel_off = 0; >> + errno = 0; >> return NULL; >> } >> @@ -339,6 +340,7 @@ static const char *lu_binary_save(const void *ctx, >> struct connection *conn, >> memcpy(lu_status->kernel + lu_status->kernel_off, data, size); >> lu_status->kernel_off += size; >> + errno = 0; >> return NULL; >> } > > I forgot to update lu_binary(). I will respin the patch once I get some > feedback. With setting errno to 0 before returning NULL in lu_binary() you can add Reviewed-by: Juergen Gross <jgross@suse.com> Juergen
Hi Juergen, On 30/07/2021 09:40, Juergen Gross wrote: > On 29.07.21 17:23, Julien Grall wrote: >> >> >> On 29/07/2021 12:06, Julien Grall wrote: >>> From: Julien Grall <jgrall@amazon.com> >>> >>> At the moment, do_control_lu() will set errno to 0 before calling >>> lu_arch() and then check errno. The expectation is nothing in lu_arch() >>> will change the value unless there is an error. >>> >>> However, per errno(3), a function that succeeds is allowed to change >>> errno. In fact, syslog() will overwrite errno if the logs are rotated >>> at the time it is called. >>> >>> To prevent any further issue, errno is now always set before >>> returning NULL. >>> >>> Additionally, errno is only checked when returning NULL so the client >>> can see the error message if there is any. >>> >>> Reported-by: Michael Kurth <mku@amazon.com> >>> Signed-off-by: Julien Grall <jgrall@amazon.com> >>> --- >>> tools/xenstore/xenstored_control.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/tools/xenstore/xenstored_control.c >>> b/tools/xenstore/xenstored_control.c >>> index 6b68b79faac7..6fcb42095b59 100644 >>> --- a/tools/xenstore/xenstored_control.c >>> +++ b/tools/xenstore/xenstored_control.c >>> @@ -324,6 +324,7 @@ static const char *lu_binary_alloc(const void >>> *ctx, struct connection *conn, >>> lu_status->kernel_size = size; >>> lu_status->kernel_off = 0; >>> + errno = 0; >>> return NULL; >>> } >>> @@ -339,6 +340,7 @@ static const char *lu_binary_save(const void >>> *ctx, struct connection *conn, >>> memcpy(lu_status->kernel + lu_status->kernel_off, data, size); >>> lu_status->kernel_off += size; >>> + errno = 0; >>> return NULL; >>> } >> >> I forgot to update lu_binary(). I will respin the patch once I get >> some feedback. > > With setting errno to 0 before returning NULL in lu_binary() you can add > > Reviewed-by: Juergen Gross <jgross@suse.com> Thanks! I will commit it. Cheers,
diff --git a/tools/xenstore/xenstored_control.c b/tools/xenstore/xenstored_control.c index 6b68b79faac7..6fcb42095b59 100644 --- a/tools/xenstore/xenstored_control.c +++ b/tools/xenstore/xenstored_control.c @@ -324,6 +324,7 @@ static const char *lu_binary_alloc(const void *ctx, struct connection *conn, lu_status->kernel_size = size; lu_status->kernel_off = 0; + errno = 0; return NULL; } @@ -339,6 +340,7 @@ static const char *lu_binary_save(const void *ctx, struct connection *conn, memcpy(lu_status->kernel + lu_status->kernel_off, data, size); lu_status->kernel_off += size; + errno = 0; return NULL; } @@ -798,9 +800,8 @@ static int do_control_lu(void *ctx, struct connection *conn, if (!ret) return errno; } else { - errno = 0; ret = lu_arch(ctx, conn, vec, num); - if (errno) + if (!ret && errno) return errno; }