Re: [PATCH] apparmor: Add empty statement between label and declaration in profile_transition(()

[John Johansen <john.johansen@canonical.com>]

On 11/11/24 06:49, Nathan Chancellor wrote:
> Clang 18 and newer warns (or errors with CONFIG_WERROR=y):
> 
>    security/apparmor/domain.c:695:3: error: label followed by a declaration is a C23 extension [-Werror,-Wc23-extensions]
>      695 |                 struct aa_profile *new_profile = NULL;
>          |                 ^
> 
> With Clang 17 and older, this is just an unconditional hard error:
> 
>    security/apparmor/domain.c:695:3: error: expected expression
>      695 |                 struct aa_profile *new_profile = NULL;
>          |                 ^
>    security/apparmor/domain.c:697:3: error: use of undeclared identifier 'new_profile'
>      697 |                 new_profile = aa_new_learning_profile(profile, false, name,
>          |                 ^
>    security/apparmor/domain.c:699:8: error: use of undeclared identifier 'new_profile'
>      699 |                 if (!new_profile) {
>          |                      ^
>    security/apparmor/domain.c:704:11: error: use of undeclared identifier 'new_profile'
>      704 |                         new = &new_profile->label;
>          |                                ^
> 
> Add a semicolon directly after the label to create an empty statement,
> which keeps the original intent of the code while clearing up the
> warning/error on all clang versions.
> 
> Fixes: ee650b3820f3 ("apparmor: properly handle cx/px lookup failure for complain")
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202411101808.AI8YG6cs-lkp@intel.com/
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>

thanks for the patch, but I went with an alternate version, that I did last Sunday
(sorry I guess I forgot to push the tree). Since I hadn't pushed the tree I did
consider replacing my patch with it but in the end decided to not go with
the C99 variable declaration, moving the var to the top of the outer block (what
my Sunday patch did).

The reason being that while I don't think the style guideline forbid them, I end
up getting patches for them anyways, as some compiler flag sets will warn about
them.




> ---
>   security/apparmor/domain.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index 602d7a1bb44823a9b81e34d270b03c5f3aff3a34..eb0f222aa29442686b0a6751001c879f5b366c59 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -691,7 +691,7 @@ static struct aa_label *profile_transition(const struct cred *subj_cred,
>   			error = -EACCES;
>   		}
>   	} else if (COMPLAIN_MODE(profile)) {
> -create_learning_profile:
> +create_learning_profile:;
>   		/* no exec permission - learning mode */
>   		struct aa_profile *new_profile = NULL;
>   
> 
> ---
> base-commit: 8c4f7960ae8a7a03a43f814e4af471b8e6ea3391
> change-id: 20241111-apparmor-fix-label-declaration-warning-fcd24ce2d447
> 
> Best regards,