| Message ID | 20221101152842.4257-16-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools/xenstore: do some cleanup and fixes | expand |
Hi Juergen, On 01/11/2022 15:28, Juergen Gross wrote: > clang is complaining about a NULL dereference for constructs like: > > domain_is_unprivileged(conn) ? conn->in : NULL I have just build xenstored with clang 11 and didn't get a complain. So can you provide more details? Cheers,
On 01.12.22 23:05, Julien Grall wrote: > Hi Juergen, > > On 01/11/2022 15:28, Juergen Gross wrote: >> clang is complaining about a NULL dereference for constructs like: >> >> domain_is_unprivileged(conn) ? conn->in : NULL > > I have just build xenstored with clang 11 and didn't get a complain. So can you > provide more details? It was reported by Edwin during development of the XSA series: On 11/08/2022 19:01, Edwin Torok wrote: > xenstored_watch.c:152:39: warning: Access to field 'in' results in a dereference of a null pointer (loaded from variable 'conn') [core.NullDereference] > req = domain_is_unprivileged(conn) ? conn->in : NULL; > ^~~~~~~~ > 1 warning generated. > > clang 14 says this is a NULL dereference ... You even responded to that report, BTW. Juergen
Hi, On 13/12/2022 07:57, Juergen Gross wrote: > On 01.12.22 23:05, Julien Grall wrote: >> Hi Juergen, >> >> On 01/11/2022 15:28, Juergen Gross wrote: >>> clang is complaining about a NULL dereference for constructs like: >>> >>> domain_is_unprivileged(conn) ? conn->in : NULL >> >> I have just build xenstored with clang 11 and didn't get a complain. >> So can you provide more details? > > It was reported by Edwin during development of the XSA series: > > On 11/08/2022 19:01, Edwin Torok wrote: > > xenstored_watch.c:152:39: warning: Access to field 'in' results in a > dereference of a null pointer (loaded from variable 'conn') > [core.NullDereference] > > req = domain_is_unprivileged(conn) ? conn->in : NULL; > > ^~~~~~~~ > > 1 warning generated. > > > > clang 14 says this is a NULL dereference ... > > You even responded to that report, BTW. I respond to a lot of e-mails and don't necessarily remember all of them :). That's why we have commit message. In this case, you want to mention the compiler version in the commit message. With that: Acked-by: Julien Grall <jgrall@amazon.com> Cheers,
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h index 37006d508d..3c4e27d0dd 100644 --- a/tools/xenstore/xenstored_core.h +++ b/tools/xenstore/xenstored_core.h @@ -297,6 +297,16 @@ void unmap_xenbus(void *interface); static inline int xenbus_master_domid(void) { return dom0_domid; } +static inline bool domid_is_unprivileged(unsigned int domid) +{ + return domid != dom0_domid && domid != priv_domid; +} + +static inline bool domain_is_unprivileged(const struct connection *conn) +{ + return conn && domid_is_unprivileged(conn->id); +} + /* Return the event channel used by xenbus. */ evtchn_port_t xenbus_evtchn(void); diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c index 0f1c903ee6..368b9bc2b7 100644 --- a/tools/xenstore/xenstored_domain.c +++ b/tools/xenstore/xenstored_domain.c @@ -412,17 +412,6 @@ void handle_event(void) barf_perror("Failed to write to event fd"); } -static bool domid_is_unprivileged(unsigned int domid) -{ - return domid != dom0_domid && domid != priv_domid; -} - -bool domain_is_unprivileged(struct connection *conn) -{ - return conn && conn->domain && - domid_is_unprivileged(conn->domain->domid); -} - static char *talloc_domain_path(const void *context, unsigned int domid) { return talloc_asprintf(context, "/local/domain/%u", domid); diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h index 1e402f2609..22996e2576 100644 --- a/tools/xenstore/xenstored_domain.h +++ b/tools/xenstore/xenstored_domain.h @@ -59,8 +59,6 @@ void ignore_connection(struct connection *conn, unsigned int err); /* Returns the implicit path of a connection (only domains have this) */ const char *get_implicit_path(const struct connection *conn); -bool domain_is_unprivileged(struct connection *conn); - /* Remove node permissions for no longer existing domains. */ int domain_adjust_node_perms(struct node *node); int domain_alloc_permrefs(struct node_perms *perms);
clang is complaining about a NULL dereference for constructs like: domain_is_unprivileged(conn) ? conn->in : NULL as it can't know that domain_is_unprivileged(conn) will return false if conn is NULL. Fix that by making domain_is_unprivileged() an inline function (and related to that domid_is_unprivileged(), too). In order not having to make struct domain public, use conn->id instead of conn->domain->domid for the test. Signed-off-by: Juergen Gross <jgross@suse.com> --- tools/xenstore/xenstored_core.h | 10 ++++++++++ tools/xenstore/xenstored_domain.c | 11 ----------- tools/xenstore/xenstored_domain.h | 2 -- 3 files changed, 10 insertions(+), 13 deletions(-)