* [PATCH] libsemanage: refactor semanage_user_roles
@ 2025-08-01 14:33 Rahul Sandhu
2025-08-01 18:26 ` Stephen Smalley
0 siblings, 1 reply; 3+ messages in thread
From: Rahul Sandhu @ 2025-08-01 14:33 UTC (permalink / raw)
To: selinux; +Cc: Rahul Sandhu
Reduce the levels of nesting by early returning on errors.
Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
---
libsemanage/src/seusers_local.c | 76 +++++++++++++++++++--------------
1 file changed, 45 insertions(+), 31 deletions(-)
diff --git a/libsemanage/src/seusers_local.c b/libsemanage/src/seusers_local.c
index eb3f82bc..282d56fa 100644
--- a/libsemanage/src/seusers_local.c
+++ b/libsemanage/src/seusers_local.c
@@ -18,39 +18,53 @@ typedef struct semanage_seuser record_t;
#include "string.h"
#include <stdlib.h>
-static char *semanage_user_roles(semanage_handle_t * handle, const char *sename) {
+static char *semanage_user_roles(semanage_handle_t * handle, const char * sename) {
char *roles = NULL;
- unsigned int num_roles;
- size_t i;
- size_t size = 0;
- const char **roles_arr;
+ const char **roles_arr = NULL;
semanage_user_key_t *key = NULL;
- semanage_user_t * user;
- if (semanage_user_key_create(handle, sename, &key) >= 0) {
- if (semanage_user_query(handle, key, &user) >= 0) {
- if (semanage_user_get_roles(handle,
- user,
- &roles_arr,
- &num_roles) >= 0) {
- for (i = 0; i<num_roles; i++) {
- size += (strlen(roles_arr[i]) + 1);
- }
- if (num_roles == 0) {
- roles = strdup("");
- } else {
- roles = malloc(size);
- if (roles) {
- strcpy(roles,roles_arr[0]);
- for (i = 1; i<num_roles; i++) {
- strcat(roles,",");
- strcat(roles,roles_arr[i]);
- }
- }
- }
- free(roles_arr);
- }
- semanage_user_free(user);
- }
+ semanage_user_t *user = NULL;
+
+ if (semanage_user_key_create(handle, sename, &key) < 0) {
+ goto cleanup;
+ }
+
+ if (semanage_user_query(handle, key, &user) < 0) {
+ goto cleanup;
+ }
+
+ unsigned int num_roles = 0;
+ if (semanage_user_get_roles(handle, user, &roles_arr, &num_roles) < 0) {
+ goto cleanup;
+ }
+
+ if (num_roles == 0) {
+ roles = strdup("");
+ goto cleanup;
+ }
+
+ size_t size = 0;
+ for (size_t i = 0; i < num_roles; i++) {
+ size += (strlen(roles_arr[i]) + 1);
+ }
+
+ roles = malloc(size);
+ if (!roles) {
+ goto cleanup;
+ }
+
+ strcpy(roles, roles_arr[0]);
+
+ for (size_t i = 1; i < num_roles; i++) {
+ strcat(roles, ",");
+ strcat(roles, roles_arr[i]);
+ }
+
+cleanup:
+ free(roles_arr);
+ if (user) {
+ semanage_user_free(user);
+ }
+ if (key) {
semanage_user_key_free(key);
}
return roles;
--
2.50.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] libsemanage: refactor semanage_user_roles
2025-08-01 14:33 [PATCH] libsemanage: refactor semanage_user_roles Rahul Sandhu
@ 2025-08-01 18:26 ` Stephen Smalley
2025-08-01 19:34 ` William Roberts
0 siblings, 1 reply; 3+ messages in thread
From: Stephen Smalley @ 2025-08-01 18:26 UTC (permalink / raw)
To: Rahul Sandhu; +Cc: selinux
On Fri, Aug 1, 2025 at 10:33 AM Rahul Sandhu <nvraxn@gmail.com> wrote:
>
> Reduce the levels of nesting by early returning on errors.
>
> Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
> ---
> libsemanage/src/seusers_local.c | 76 +++++++++++++++++++--------------
> 1 file changed, 45 insertions(+), 31 deletions(-)
>
> diff --git a/libsemanage/src/seusers_local.c b/libsemanage/src/seusers_local.c
> index eb3f82bc..282d56fa 100644
> --- a/libsemanage/src/seusers_local.c
> +++ b/libsemanage/src/seusers_local.c
> @@ -18,39 +18,53 @@ typedef struct semanage_seuser record_t;
> #include "string.h"
> #include <stdlib.h>
>
> -static char *semanage_user_roles(semanage_handle_t * handle, const char *sename) {
> +static char *semanage_user_roles(semanage_handle_t * handle, const char * sename) {
> char *roles = NULL;
> - unsigned int num_roles;
> - size_t i;
> - size_t size = 0;
> - const char **roles_arr;
> + const char **roles_arr = NULL;
> semanage_user_key_t *key = NULL;
> - semanage_user_t * user;
> - if (semanage_user_key_create(handle, sename, &key) >= 0) {
> - if (semanage_user_query(handle, key, &user) >= 0) {
> - if (semanage_user_get_roles(handle,
> - user,
> - &roles_arr,
> - &num_roles) >= 0) {
> - for (i = 0; i<num_roles; i++) {
> - size += (strlen(roles_arr[i]) + 1);
> - }
> - if (num_roles == 0) {
> - roles = strdup("");
> - } else {
> - roles = malloc(size);
> - if (roles) {
> - strcpy(roles,roles_arr[0]);
> - for (i = 1; i<num_roles; i++) {
> - strcat(roles,",");
> - strcat(roles,roles_arr[i]);
> - }
> - }
> - }
> - free(roles_arr);
> - }
> - semanage_user_free(user);
> - }
> + semanage_user_t *user = NULL;
> +
> + if (semanage_user_key_create(handle, sename, &key) < 0) {
> + goto cleanup;
> + }
Here and below, no need for { } around a single statement body.
> +
> + if (semanage_user_query(handle, key, &user) < 0) {
> + goto cleanup;
> + }
> +
> + unsigned int num_roles = 0;
> + if (semanage_user_get_roles(handle, user, &roles_arr, &num_roles) < 0) {
> + goto cleanup;
> + }
> +
> + if (num_roles == 0) {
> + roles = strdup("");
> + goto cleanup;
> + }
> +
> + size_t size = 0;
> + for (size_t i = 0; i < num_roles; i++) {
> + size += (strlen(roles_arr[i]) + 1);
> + }
> +
> + roles = malloc(size);
> + if (!roles) {
> + goto cleanup;
> + }
> +
> + strcpy(roles, roles_arr[0]);
> +
> + for (size_t i = 1; i < num_roles; i++) {
> + strcat(roles, ",");
> + strcat(roles, roles_arr[i]);
> + }
> +
> +cleanup:
> + free(roles_arr);
> + if (user) {
> + semanage_user_free(user);
> + }
Here and below, looks like it is valid to call with a NULL user/key so
no need to test for non-NULL.
> + if (key) {
> semanage_user_key_free(key);
> }
> return roles;
> --
> 2.50.1
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] libsemanage: refactor semanage_user_roles
2025-08-01 18:26 ` Stephen Smalley
@ 2025-08-01 19:34 ` William Roberts
0 siblings, 0 replies; 3+ messages in thread
From: William Roberts @ 2025-08-01 19:34 UTC (permalink / raw)
To: Stephen Smalley; +Cc: Rahul Sandhu, selinux
On Fri, Aug 1, 2025 at 1:27 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Fri, Aug 1, 2025 at 10:33 AM Rahul Sandhu <nvraxn@gmail.com> wrote:
> >
> > Reduce the levels of nesting by early returning on errors.
> >
> > Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
> > ---
> > libsemanage/src/seusers_local.c | 76 +++++++++++++++++++--------------
> > 1 file changed, 45 insertions(+), 31 deletions(-)
> >
> > diff --git a/libsemanage/src/seusers_local.c b/libsemanage/src/seusers_local.c
> > index eb3f82bc..282d56fa 100644
> > --- a/libsemanage/src/seusers_local.c
> > +++ b/libsemanage/src/seusers_local.c
> > @@ -18,39 +18,53 @@ typedef struct semanage_seuser record_t;
> > #include "string.h"
> > #include <stdlib.h>
> >
> > -static char *semanage_user_roles(semanage_handle_t * handle, const char *sename) {
> > +static char *semanage_user_roles(semanage_handle_t * handle, const char * sename) {
> > char *roles = NULL;
> > - unsigned int num_roles;
> > - size_t i;
> > - size_t size = 0;
> > - const char **roles_arr;
> > + const char **roles_arr = NULL;
> > semanage_user_key_t *key = NULL;
> > - semanage_user_t * user;
> > - if (semanage_user_key_create(handle, sename, &key) >= 0) {
> > - if (semanage_user_query(handle, key, &user) >= 0) {
> > - if (semanage_user_get_roles(handle,
> > - user,
> > - &roles_arr,
> > - &num_roles) >= 0) {
> > - for (i = 0; i<num_roles; i++) {
> > - size += (strlen(roles_arr[i]) + 1);
> > - }
> > - if (num_roles == 0) {
> > - roles = strdup("");
> > - } else {
> > - roles = malloc(size);
> > - if (roles) {
> > - strcpy(roles,roles_arr[0]);
> > - for (i = 1; i<num_roles; i++) {
> > - strcat(roles,",");
> > - strcat(roles,roles_arr[i]);
> > - }
> > - }
> > - }
> > - free(roles_arr);
> > - }
> > - semanage_user_free(user);
> > - }
> > + semanage_user_t *user = NULL;
> > +
> > + if (semanage_user_key_create(handle, sename, &key) < 0) {
> > + goto cleanup;
> > + }
>
> Here and below, no need for { } around a single statement body.
>
> > +
> > + if (semanage_user_query(handle, key, &user) < 0) {
> > + goto cleanup;
> > + }
> > +
> > + unsigned int num_roles = 0;
> > + if (semanage_user_get_roles(handle, user, &roles_arr, &num_roles) < 0) {
> > + goto cleanup;
> > + }
> > +
> > + if (num_roles == 0) {
> > + roles = strdup("");
> > + goto cleanup;
> > + }
> > +
> > + size_t size = 0;
> > + for (size_t i = 0; i < num_roles; i++) {
> > + size += (strlen(roles_arr[i]) + 1);
> > + }
> > +
> > + roles = malloc(size);
> > + if (!roles) {
> > + goto cleanup;
> > + }
> > +
> > + strcpy(roles, roles_arr[0]);
> > +
> > + for (size_t i = 1; i < num_roles; i++) {
> > + strcat(roles, ",");
> > + strcat(roles, roles_arr[i]);
> > + }
> > +
> > +cleanup:
> > + free(roles_arr);
> > + if (user) {
> > + semanage_user_free(user);
> > + }
>
> Here and below, looks like it is valid to call with a NULL user/key so
> no need to test for non-NULL.
IIUC, there are also jumps to the cleanup label where variable user
could be uninitialized as well, so we need user initialized to NULL.
>
> > + if (key) {
> > semanage_user_key_free(key);
> > }
> > return roles;
> > --
> > 2.50.1
> >
> >
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-08-01 19:34 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-01 14:33 [PATCH] libsemanage: refactor semanage_user_roles Rahul Sandhu
2025-08-01 18:26 ` Stephen Smalley
2025-08-01 19:34 ` William Roberts
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).