| Message ID | alpine.DEB.2.22.394.2212061808570.4039@ubuntu-linux-20-04-desktop (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xsm: misra rule 8.4 fix | expand |
On 07.12.2022 03:12, Stefano Stabellini wrote: > Fix two MISRA Issues Rule 8.4 ("A compatible declaration shall be > visible when an object or function with external linkage is defined") > found by cppcheck affecting xen/xsm/flask/ss/services.c. > > Fix the first issue by making policydb_loaded_version static This variable is only ever written to afaics, so perhaps better simply drop it? > and the > second issue by declaring ss_initialized in a proper header. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> As to the title: The changes are contained to Flask, so xsm: really is too wide a prefix. > --- a/xen/xsm/flask/flask_op.c > +++ b/xen/xsm/flask/flask_op.c > @@ -56,8 +56,6 @@ static int bool_num = 0; > static int *bool_pending_values = NULL; > static int flask_security_make_bools(void); > > -extern int ss_initialized; What about the 2nd one in flask/ss/policydb.c? Jan
On 12/7/22 04:47, Jan Beulich wrote: > On 07.12.2022 03:12, Stefano Stabellini wrote: >> Fix two MISRA Issues Rule 8.4 ("A compatible declaration shall be >> visible when an object or function with external linkage is defined") >> found by cppcheck affecting xen/xsm/flask/ss/services.c. >> >> Fix the first issue by making policydb_loaded_version static > > This variable is only ever written to afaics, so perhaps better simply > drop it? I agree, I would ack a patch with this dropped. >> and the >> second issue by declaring ss_initialized in a proper header. >> >> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > > As to the title: The changes are contained to Flask, so xsm: really > is too wide a prefix. > >> --- a/xen/xsm/flask/flask_op.c >> +++ b/xen/xsm/flask/flask_op.c >> @@ -56,8 +56,6 @@ static int bool_num = 0; >> static int *bool_pending_values = NULL; >> static int flask_security_make_bools(void); >> >> -extern int ss_initialized; > > What about the 2nd one in flask/ss/policydb.c? > > Jan dps
Hi Stefano, On 07/12/2022 03:12, Stefano Stabellini wrote: > > > Fix two MISRA Issues Rule 8.4 ("A compatible declaration shall be > visible when an object or function with external linkage is defined") > found by cppcheck affecting xen/xsm/flask/ss/services.c. > > Fix the first issue by making policydb_loaded_version static and the > second issue by declaring ss_initialized in a proper header. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> cppcheck also reports findings for rule 8.4 with regards to the following functions: - security_get_bools - security_set_bools - security_get_bool_value - security_get_bool_name The prototypes for them are stored in xen/xsm/flask/include/conditional.h, but services.c only does: #include "conditional.h" This include refers to xen/xsm/flask/ss/conditional.h and not to xen/xsm/flask/include/conditional.h. This means that we should also explicitly do: #include <conditional.h> This way we will fix all the findings in this file for the rule 8.4. ~Michal
On 07.12.2022 13:33, Michal Orzel wrote: > Hi Stefano, > > On 07/12/2022 03:12, Stefano Stabellini wrote: >> >> >> Fix two MISRA Issues Rule 8.4 ("A compatible declaration shall be >> visible when an object or function with external linkage is defined") >> found by cppcheck affecting xen/xsm/flask/ss/services.c. >> >> Fix the first issue by making policydb_loaded_version static and the >> second issue by declaring ss_initialized in a proper header. >> >> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > > cppcheck also reports findings for rule 8.4 with regards to the following functions: > - security_get_bools > - security_set_bools > - security_get_bool_value > - security_get_bool_name > > The prototypes for them are stored in xen/xsm/flask/include/conditional.h, > but services.c only does: > #include "conditional.h" > > This include refers to xen/xsm/flask/ss/conditional.h and not to xen/xsm/flask/include/conditional.h. > This means that we should also explicitly do: > #include <conditional.h> And Misra has no rule disallowing such use of two different, identically named headers, for being potentially ambiguous? Jan
On 08/12/2022 09:14, Jan Beulich wrote: > > > On 07.12.2022 13:33, Michal Orzel wrote: >> Hi Stefano, >> >> On 07/12/2022 03:12, Stefano Stabellini wrote: >>> >>> >>> Fix two MISRA Issues Rule 8.4 ("A compatible declaration shall be >>> visible when an object or function with external linkage is defined") >>> found by cppcheck affecting xen/xsm/flask/ss/services.c. >>> >>> Fix the first issue by making policydb_loaded_version static and the >>> second issue by declaring ss_initialized in a proper header. >>> >>> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> >> >> cppcheck also reports findings for rule 8.4 with regards to the following functions: >> - security_get_bools >> - security_set_bools >> - security_get_bool_value >> - security_get_bool_name >> >> The prototypes for them are stored in xen/xsm/flask/include/conditional.h, >> but services.c only does: >> #include "conditional.h" >> >> This include refers to xen/xsm/flask/ss/conditional.h and not to xen/xsm/flask/include/conditional.h. >> This means that we should also explicitly do: >> #include <conditional.h> > > And Misra has no rule disallowing such use of two different, identically > named headers, for being potentially ambiguous? I could not find such rule/directive related to filenames; only \wrt identifiers. But all in all, we do not need MISRA to modify the filenames to get rid of ambiguity if we really want to. > > Jan ~Michal
On 08.12.2022 09:14, Jan Beulich wrote: > On 07.12.2022 13:33, Michal Orzel wrote: >> On 07/12/2022 03:12, Stefano Stabellini wrote: >>> Fix two MISRA Issues Rule 8.4 ("A compatible declaration shall be >>> visible when an object or function with external linkage is defined") >>> found by cppcheck affecting xen/xsm/flask/ss/services.c. >>> >>> Fix the first issue by making policydb_loaded_version static and the >>> second issue by declaring ss_initialized in a proper header. >>> >>> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> >> >> cppcheck also reports findings for rule 8.4 with regards to the following functions: >> - security_get_bools >> - security_set_bools >> - security_get_bool_value >> - security_get_bool_name >> >> The prototypes for them are stored in xen/xsm/flask/include/conditional.h, >> but services.c only does: >> #include "conditional.h" >> >> This include refers to xen/xsm/flask/ss/conditional.h and not to xen/xsm/flask/include/conditional.h. >> This means that we should also explicitly do: >> #include <conditional.h> > > And Misra has no rule disallowing such use of two different, identically > named headers, for being potentially ambiguous? Actually this is even more fragile than I thought when sending the above question, albeit still working correctly as per gcc implementation defined behavior. The further weakness stems from CFLAGS-y += -iquote $(objtree)/xsm/flask/include CFLAGS-y += -I$(srctree)/xsm/flask/include which was added for out-of-tree builds, rendering in-tree builds search for #include "..." files in xsm/flask/include too early. The only thing that saves us is that the current directory is searched yet earlier. However, as per gcc implementation defined behavior "#include <conditional.h>" is likely wrong to use anyway, as this header can in no way be considered a "system header"; it clearly falls under "header files of your own program", where "own program" is Flask here. For being a system header the file ought to live in include/xen/ or include/xsm/ (and accordingly be included via "#include <xen/conditional.h>" or "#include <xsm/conditional.h>"), potentially in a respective subdir there. My view is that these "#include <...>" (there are more, albeit non-ambiguous ones) all want to be converted to '#include "..."'. That'll then also eliminate the ambiguity with conditional.h (as one will then [need to] come with a path prefix). Jan
diff --git a/xen/xsm/flask/flask_op.c b/xen/xsm/flask/flask_op.c index d319466c6b..b866e8d05f 100644 --- a/xen/xsm/flask/flask_op.c +++ b/xen/xsm/flask/flask_op.c @@ -56,8 +56,6 @@ static int bool_num = 0; static int *bool_pending_values = NULL; static int flask_security_make_bools(void); -extern int ss_initialized; - static int __init cf_check parse_flask_param(const char *s) { if ( !strcmp(s, "enforcing") ) diff --git a/xen/xsm/flask/private.h b/xen/xsm/flask/private.h index 429f213cce..2e0e65489c 100644 --- a/xen/xsm/flask/private.h +++ b/xen/xsm/flask/private.h @@ -6,4 +6,6 @@ long cf_check do_flask_op(XEN_GUEST_HANDLE_PARAM(void) u_flask_op); int cf_check compat_flask_op(XEN_GUEST_HANDLE_PARAM(void) u_flask_op); +extern int ss_initialized; + #endif /* XSM_FLASK_PRIVATE */ diff --git a/xen/xsm/flask/ss/services.c b/xen/xsm/flask/ss/services.c index dab07b5f60..4665f3b007 100644 --- a/xen/xsm/flask/ss/services.c +++ b/xen/xsm/flask/ss/services.c @@ -53,7 +53,7 @@ #include "conditional.h" #include "mls.h" -unsigned int policydb_loaded_version; +static unsigned int policydb_loaded_version; static DEFINE_RWLOCK(policy_rwlock); #define POLICY_RDLOCK read_lock(&policy_rwlock)
Fix two MISRA Issues Rule 8.4 ("A compatible declaration shall be visible when an object or function with external linkage is defined") found by cppcheck affecting xen/xsm/flask/ss/services.c. Fix the first issue by making policydb_loaded_version static and the second issue by declaring ss_initialized in a proper header. Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>