[PATCH] genhomedircon: cleanup parsing of uid config values

[PATCH] genhomedircon: cleanup parsing of uid config values

[Rahul Sandhu]

Parsing KV files with a separator of similar format is fairly similar,
so we may as well add a helper function to make it easier to read.

Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
---
 libsemanage/src/genhomedircon.c | 81 +++++++++++++--------------------
 1 file changed, 32 insertions(+), 49 deletions(-)

diff --git a/libsemanage/src/genhomedircon.c b/libsemanage/src/genhomedircon.c
index 34056562..08a22df1 100644
--- a/libsemanage/src/genhomedircon.c
+++ b/libsemanage/src/genhomedircon.c
@@ -308,6 +308,29 @@ done:
 	return retval;
 }
 
+static int parse_uid_config(const char *file, const char *key, const char *sep,
+		uid_t fallback, uid_t *out)
+{
+	char *path = semanage_findval(file, key, sep);
+	if (!path || !*path) {
+		free(path);
+		*out = fallback;
+		return 0;
+	}
+
+	char *endptr;
+	const unsigned long val = strtoul(path, &endptr, 0);
+	free(path);
+
+	if (endptr != path && *endptr == '\0') {
+		*out = (uid_t)val;
+		return 0;
+	}
+
+	*out = fallback;
+	return -1;
+}
+
 static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
 {
 	semanage_list_t *homedir_list = NULL;
@@ -315,7 +338,6 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
 	fc_match_handle_t hand;
 	char *path = NULL;
 	uid_t temp, minuid = 500, maxuid = 60000;
-	int minuid_set = 0;
 	struct passwd *pwbuf;
 	struct stat buf;
 
@@ -362,56 +384,17 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
 	     "Conversion failed for key " key ", is its value a number?" \
 	     "  Falling back to default value of `%s`.", #val);
 
-	path = semanage_findval(PATH_ETC_LOGIN_DEFS, "UID_MIN", NULL);
-	if (path && *path) {
-		char *endptr;
-		const unsigned long val = strtoul(path, &endptr, 0);
-		if (endptr != path && *endptr == '\0') {
-			minuid = (uid_t)val;
-			minuid_set = 1;
-		} else {
-			/* we were provided an invalid value, use defaults.  */
-			genhomedircon_warn_conv_fail("UID_MIN", FALLBACK_MINUID);
-			minuid = FALLBACK_MINUID;
-			minuid_set = 1;
-		}
-	}
-	free(path);
-	path = NULL;
+	if (parse_uid_config(PATH_ETC_LOGIN_DEFS, "UID_MIN", NULL, FALLBACK_MINUID, &minuid) < 0)
+		genhomedircon_warn_conv_fail("UID_MIN", FALLBACK_MINUID);
 
-	path = semanage_findval(PATH_ETC_LOGIN_DEFS, "UID_MAX", NULL);
-	if (path && *path) {
-		char *endptr;
-		const unsigned long val = strtoul(path, &endptr, 0);
-		if (endptr != path && *endptr == '\0') {
-			maxuid = (uid_t)val;
-		} else {
-			/* we were provided an invalid value, use defaults.  */
-			genhomedircon_warn_conv_fail("UID_MAX", FALLBACK_MAXUID);
-			maxuid = FALLBACK_MAXUID;
-		}
-	}
-	free(path);
-	path = NULL;
+	if (parse_uid_config(PATH_ETC_LOGIN_DEFS, "UID_MAX", NULL, FALLBACK_MAXUID, &maxuid) < 0)
+		genhomedircon_warn_conv_fail("UID_MAX", FALLBACK_MAXUID);
 
-	path = semanage_findval(PATH_ETC_LIBUSER, "LU_UIDNUMBER", "=");
-	if (path && *path) {
-		char *endptr;
-		const unsigned long val = strtoul(path, &endptr, 0);
-		if (endptr != path && *endptr == '\0') {
-			temp = (uid_t)val;
-		} else {
-			/* we were provided an invalid value, use defaults.  */
-			genhomedircon_warn_conv_fail("LU_UIDNUMBER", FALLBACK_LU_UIDNUMBER);
-			temp = FALLBACK_LU_UIDNUMBER;
-		}
-		if (!minuid_set || temp < minuid) {
-			minuid = temp;
-			minuid_set = 1;
-		}
-	}
-	free(path);
-	path = NULL;
+	if (parse_uid_config(PATH_ETC_LIBUSER, "LU_UIDNUMBER", "=", FALLBACK_LU_UIDNUMBER, &temp) < 0)
+		genhomedircon_warn_conv_fail("LU_UIDNUMBER", FALLBACK_LU_UIDNUMBER);
+
+	if (temp < minuid)
+		minuid = temp;
 
 #undef genhomedircon_warn_conv_fail
 
-- 
2.51.0

Re: [PATCH] genhomedircon: cleanup parsing of uid config values

[Stephen Smalley]

On Tue, Oct 21, 2025 at 7:36 AM Rahul Sandhu <nvraxn@gmail.com> wrote:
>
> Parsing KV files with a separator of similar format is fairly similar,
> so we may as well add a helper function to make it easier to read.
>
> Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
> ---
>  libsemanage/src/genhomedircon.c | 81 +++++++++++++--------------------
>  1 file changed, 32 insertions(+), 49 deletions(-)
>
> diff --git a/libsemanage/src/genhomedircon.c b/libsemanage/src/genhomedircon.c
> index 34056562..08a22df1 100644
> --- a/libsemanage/src/genhomedircon.c
> +++ b/libsemanage/src/genhomedircon.c
> @@ -308,6 +308,29 @@ done:
>         return retval;
>  }
>
> +static int parse_uid_config(const char *file, const char *key, const char *sep,
> +               uid_t fallback, uid_t *out)
> +{
> +       char *path = semanage_findval(file, key, sep);

Not new to this patch, but more obvious now that it is in a dedicated
helper: seems odd to call this path when it is a UID string.

> +       if (!path || !*path) {
> +               free(path);
> +               *out = fallback;
> +               return 0;
> +       }
> +
> +       char *endptr;
> +       const unsigned long val = strtoul(path, &endptr, 0);
> +       free(path);
> +
> +       if (endptr != path && *endptr == '\0') {

This could end up dereferencing the memory you freed above.

[PATCH v2] genhomedircon: cleanup parsing of uid config values

[Rahul Sandhu]

Parsing KV files with a separator of similar format is fairly similar,
so we may as well add a helper function to make it easier to read.

Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
---
 libsemanage/src/genhomedircon.c | 82 +++++++++++++--------------------
 1 file changed, 33 insertions(+), 49 deletions(-)

v2: rename path to something more sensible (afterall, we are parsing a
    UID!) and move the free to later, just before both return paths to
    not dereference it when checking whether we actually parsed a valid
    number or not.

diff --git a/libsemanage/src/genhomedircon.c b/libsemanage/src/genhomedircon.c
index 34056562..bb840856 100644
--- a/libsemanage/src/genhomedircon.c
+++ b/libsemanage/src/genhomedircon.c
@@ -308,6 +308,30 @@ done:
 	return retval;
 }
 
+static int parse_uid_config(const char *file, const char *key, const char *sep,
+		uid_t fallback, uid_t *out)
+{
+	char *uid_str = semanage_findval(file, key, sep);
+	if (!uid_str || !*uid_str) {
+		free(uid_str);
+		*out = fallback;
+		return 0;
+	}
+
+	char *endptr;
+	const unsigned long val = strtoul(uid_str, &endptr, 0);
+
+	if (endptr != uid_str && *endptr == '\0') {
+		*out = (uid_t)val;
+		free(uid_str);
+		return 0;
+	}
+
+	free(uid_str);
+	*out = fallback;
+	return -1;
+}
+
 static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
 {
 	semanage_list_t *homedir_list = NULL;
@@ -315,7 +339,6 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
 	fc_match_handle_t hand;
 	char *path = NULL;
 	uid_t temp, minuid = 500, maxuid = 60000;
-	int minuid_set = 0;
 	struct passwd *pwbuf;
 	struct stat buf;
 
@@ -362,56 +385,17 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
 	     "Conversion failed for key " key ", is its value a number?" \
 	     "  Falling back to default value of `%s`.", #val);
 
-	path = semanage_findval(PATH_ETC_LOGIN_DEFS, "UID_MIN", NULL);
-	if (path && *path) {
-		char *endptr;
-		const unsigned long val = strtoul(path, &endptr, 0);
-		if (endptr != path && *endptr == '\0') {
-			minuid = (uid_t)val;
-			minuid_set = 1;
-		} else {
-			/* we were provided an invalid value, use defaults.  */
-			genhomedircon_warn_conv_fail("UID_MIN", FALLBACK_MINUID);
-			minuid = FALLBACK_MINUID;
-			minuid_set = 1;
-		}
-	}
-	free(path);
-	path = NULL;
+	if (parse_uid_config(PATH_ETC_LOGIN_DEFS, "UID_MIN", NULL, FALLBACK_MINUID, &minuid) < 0)
+		genhomedircon_warn_conv_fail("UID_MIN", FALLBACK_MINUID);
 
-	path = semanage_findval(PATH_ETC_LOGIN_DEFS, "UID_MAX", NULL);
-	if (path && *path) {
-		char *endptr;
-		const unsigned long val = strtoul(path, &endptr, 0);
-		if (endptr != path && *endptr == '\0') {
-			maxuid = (uid_t)val;
-		} else {
-			/* we were provided an invalid value, use defaults.  */
-			genhomedircon_warn_conv_fail("UID_MAX", FALLBACK_MAXUID);
-			maxuid = FALLBACK_MAXUID;
-		}
-	}
-	free(path);
-	path = NULL;
+	if (parse_uid_config(PATH_ETC_LOGIN_DEFS, "UID_MAX", NULL, FALLBACK_MAXUID, &maxuid) < 0)
+		genhomedircon_warn_conv_fail("UID_MAX", FALLBACK_MAXUID);
 
-	path = semanage_findval(PATH_ETC_LIBUSER, "LU_UIDNUMBER", "=");
-	if (path && *path) {
-		char *endptr;
-		const unsigned long val = strtoul(path, &endptr, 0);
-		if (endptr != path && *endptr == '\0') {
-			temp = (uid_t)val;
-		} else {
-			/* we were provided an invalid value, use defaults.  */
-			genhomedircon_warn_conv_fail("LU_UIDNUMBER", FALLBACK_LU_UIDNUMBER);
-			temp = FALLBACK_LU_UIDNUMBER;
-		}
-		if (!minuid_set || temp < minuid) {
-			minuid = temp;
-			minuid_set = 1;
-		}
-	}
-	free(path);
-	path = NULL;
+	if (parse_uid_config(PATH_ETC_LIBUSER, "LU_UIDNUMBER", "=", FALLBACK_LU_UIDNUMBER, &temp) < 0)
+		genhomedircon_warn_conv_fail("LU_UIDNUMBER", FALLBACK_LU_UIDNUMBER);
+
+	if (temp < minuid)
+		minuid = temp;
 
 #undef genhomedircon_warn_conv_fail
 
-- 
2.51.0

Re: [PATCH v2] genhomedircon: cleanup parsing of uid config values

[Stephen Smalley]

On Tue, Oct 21, 2025 at 8:01 PM Rahul Sandhu <nvraxn@gmail.com> wrote:
>
> Parsing KV files with a separator of similar format is fairly similar,
> so we may as well add a helper function to make it easier to read.
>
> Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
> ---
>  libsemanage/src/genhomedircon.c | 82 +++++++++++++--------------------
>  1 file changed, 33 insertions(+), 49 deletions(-)
>
> v2: rename path to something more sensible (afterall, we are parsing a
>     UID!) and move the free to later, just before both return paths to
>     not dereference it when checking whether we actually parsed a valid
>     number or not.
>
> diff --git a/libsemanage/src/genhomedircon.c b/libsemanage/src/genhomedircon.c
> index 34056562..bb840856 100644
> --- a/libsemanage/src/genhomedircon.c
> +++ b/libsemanage/src/genhomedircon.c
> @@ -315,7 +339,6 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
>         fc_match_handle_t hand;
>         char *path = NULL;
>         uid_t temp, minuid = 500, maxuid = 60000;

Should we be using our FALLBACK_* definitions above to initialize? Or
alternatively, do we need to initialize them at all since
parse_uid_config() always sets them?

> -       int minuid_set = 0;

Not sure you can drop this altogether, see below.

> @@ -362,56 +385,17 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
>              "Conversion failed for key " key ", is its value a number?" \
>              "  Falling back to default value of `%s`.", #val);
>
> -       path = semanage_findval(PATH_ETC_LOGIN_DEFS, "UID_MIN", NULL);
> -       if (path && *path) {
> -               char *endptr;
> -               const unsigned long val = strtoul(path, &endptr, 0);
> -               if (endptr != path && *endptr == '\0') {
> -                       minuid = (uid_t)val;
> -                       minuid_set = 1;
> -               } else {
> -                       /* we were provided an invalid value, use defaults.  */
> -                       genhomedircon_warn_conv_fail("UID_MIN", FALLBACK_MINUID);
> -                       minuid = FALLBACK_MINUID;
> -                       minuid_set = 1;
> -               }
> -       }

Note that minuid_set is NOT set to 1 if !path or !*path i.e. no
UID_MIN definition found at all in login.defs.

> -       path = semanage_findval(PATH_ETC_LIBUSER, "LU_UIDNUMBER", "=");
> -       if (path && *path) {
> -               char *endptr;
> -               const unsigned long val = strtoul(path, &endptr, 0);
> -               if (endptr != path && *endptr == '\0') {
> -                       temp = (uid_t)val;
> -               } else {
> -                       /* we were provided an invalid value, use defaults.  */
> -                       genhomedircon_warn_conv_fail("LU_UIDNUMBER", FALLBACK_LU_UIDNUMBER);
> -                       temp = FALLBACK_LU_UIDNUMBER;
> -               }
> -               if (!minuid_set || temp < minuid) {
> -                       minuid = temp;
> -                       minuid_set = 1;
> -               }
> -       }
> -       free(path);
> -       path = NULL;
> +       if (parse_uid_config(PATH_ETC_LIBUSER, "LU_UIDNUMBER", "=", FALLBACK_LU_UIDNUMBER, &temp) < 0)
> +               genhomedircon_warn_conv_fail("LU_UIDNUMBER", FALLBACK_LU_UIDNUMBER);
> +
> +       if (temp < minuid)
> +               minuid = temp;

This might lower minuid to FALLBACK_LU_UIDNUMBER (500) even if it was
explicitly set by login.defs.

[PATCH v3] genhomedircon: cleanup parsing of uid config values

[Rahul Sandhu]

Parsing KV files with a separator of similar format is fairly similar,
so we may as well add a helper function to make it easier to read.

Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
---
 libsemanage/src/genhomedircon.c | 107 +++++++++++++++++---------------
 1 file changed, 57 insertions(+), 50 deletions(-)

v2: rename path to something more sensible (afterall, we are parsing a
    UID!) and move the free to later, just before both return paths to
    not dereference it when checking whether we actually parsed a valid
    number or not.
v3: handle the fallback case for minuid properly such that we don't end
    up always using a fallback if minuid is not set in login.defs, and
    return a bool instead as it's a bit more sensible for what we're
    trying to return. Also, check for ERANGE.

diff --git a/libsemanage/src/genhomedircon.c b/libsemanage/src/genhomedircon.c
index 34056562..e91c64e6 100644
--- a/libsemanage/src/genhomedircon.c
+++ b/libsemanage/src/genhomedircon.c
@@ -308,14 +308,52 @@ done:
 	return retval;
 }
 
+/*
+ * Parses `file` for `key` seperated by `sep` into `out`.
+ * Returns:
+ *   true on success.
+ *   false on failure.
+ *   `out` is guaranteed to be initalised.
+ *   `fallback_set` is initalised to false, and set to true if a fallback was used.
+ */
+static bool parse_uid_config(const char *file, const char *key, const char *sep,
+		uid_t fallback, uid_t *out, bool *fallback_set)
+{
+	assert(out);
+	assert(fallback_set);
+
+	*fallback_set = false;
+
+	char *uid_str = semanage_findval(file, key, sep);
+	if (!uid_str || !*uid_str) {
+		free(uid_str);
+		*fallback_set = true;
+		*out = fallback;
+		return true;
+	}
+
+	char *endptr;
+	errno = 0;
+	const unsigned long val = strtoul(uid_str, &endptr, 0);
+
+	if (endptr != uid_str && *endptr == '\0' && errno != ERANGE) {
+		*out = (uid_t)val;
+		free(uid_str);
+		return true;
+	}
+
+	free(uid_str);
+	*fallback_set = true;
+	*out = fallback;
+	return false;
+}
+
 static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
 {
 	semanage_list_t *homedir_list = NULL;
 	semanage_list_t *shells = NULL;
 	fc_match_handle_t hand;
 	char *path = NULL;
-	uid_t temp, minuid = 500, maxuid = 60000;
-	int minuid_set = 0;
 	struct passwd *pwbuf;
 	struct stat buf;
 
@@ -362,56 +400,25 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
 	     "Conversion failed for key " key ", is its value a number?" \
 	     "  Falling back to default value of `%s`.", #val);
 
-	path = semanage_findval(PATH_ETC_LOGIN_DEFS, "UID_MIN", NULL);
-	if (path && *path) {
-		char *endptr;
-		const unsigned long val = strtoul(path, &endptr, 0);
-		if (endptr != path && *endptr == '\0') {
-			minuid = (uid_t)val;
-			minuid_set = 1;
-		} else {
-			/* we were provided an invalid value, use defaults.  */
-			genhomedircon_warn_conv_fail("UID_MIN", FALLBACK_MINUID);
-			minuid = FALLBACK_MINUID;
-			minuid_set = 1;
-		}
-	}
-	free(path);
-	path = NULL;
+	uid_t minuid;
+	bool fallback_set;
+	if (!parse_uid_config(PATH_ETC_LOGIN_DEFS, "UID_MIN", NULL, FALLBACK_MINUID, &minuid, &fallback_set))
+		genhomedircon_warn_conv_fail("UID_MIN", FALLBACK_MINUID);
 
-	path = semanage_findval(PATH_ETC_LOGIN_DEFS, "UID_MAX", NULL);
-	if (path && *path) {
-		char *endptr;
-		const unsigned long val = strtoul(path, &endptr, 0);
-		if (endptr != path && *endptr == '\0') {
-			maxuid = (uid_t)val;
-		} else {
-			/* we were provided an invalid value, use defaults.  */
-			genhomedircon_warn_conv_fail("UID_MAX", FALLBACK_MAXUID);
-			maxuid = FALLBACK_MAXUID;
-		}
-	}
-	free(path);
-	path = NULL;
+	const bool logindefs_minuid_fallback_set = fallback_set;
 
-	path = semanage_findval(PATH_ETC_LIBUSER, "LU_UIDNUMBER", "=");
-	if (path && *path) {
-		char *endptr;
-		const unsigned long val = strtoul(path, &endptr, 0);
-		if (endptr != path && *endptr == '\0') {
-			temp = (uid_t)val;
-		} else {
-			/* we were provided an invalid value, use defaults.  */
-			genhomedircon_warn_conv_fail("LU_UIDNUMBER", FALLBACK_LU_UIDNUMBER);
-			temp = FALLBACK_LU_UIDNUMBER;
-		}
-		if (!minuid_set || temp < minuid) {
-			minuid = temp;
-			minuid_set = 1;
-		}
-	}
-	free(path);
-	path = NULL;
+	uid_t temp;
+	if (!parse_uid_config(PATH_ETC_LIBUSER, "LU_UIDNUMBER", "=", FALLBACK_LU_UIDNUMBER, &temp, &fallback_set))
+		genhomedircon_warn_conv_fail("LU_UIDNUMBER", FALLBACK_LU_UIDNUMBER);
+
+	if (logindefs_minuid_fallback_set)
+		minuid = temp;
+
+	uid_t maxuid;
+	/* We don't actually check fallback_set here, PATH_ETC_LOGIN_DEFS is the one source of
+	   truth for UID_MAX.  */
+	if (!parse_uid_config(PATH_ETC_LOGIN_DEFS, "UID_MAX", NULL, FALLBACK_MAXUID, &maxuid, &fallback_set))
+		genhomedircon_warn_conv_fail("UID_MAX", FALLBACK_MAXUID);
 
 #undef genhomedircon_warn_conv_fail
 
-- 
2.51.0

Re: [PATCH v3] genhomedircon: cleanup parsing of uid config values

[Stephen Smalley]

On Fri, Oct 24, 2025 at 4:08 AM Rahul Sandhu <nvraxn@gmail.com> wrote:
>
> Parsing KV files with a separator of similar format is fairly similar,
> so we may as well add a helper function to make it easier to read.
>
> Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>

Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>

> ---
>  libsemanage/src/genhomedircon.c | 107 +++++++++++++++++---------------
>  1 file changed, 57 insertions(+), 50 deletions(-)
>
> v2: rename path to something more sensible (afterall, we are parsing a
>     UID!) and move the free to later, just before both return paths to
>     not dereference it when checking whether we actually parsed a valid
>     number or not.
> v3: handle the fallback case for minuid properly such that we don't end
>     up always using a fallback if minuid is not set in login.defs, and
>     return a bool instead as it's a bit more sensible for what we're
>     trying to return. Also, check for ERANGE.
>
> diff --git a/libsemanage/src/genhomedircon.c b/libsemanage/src/genhomedircon.c
> index 34056562..e91c64e6 100644
> --- a/libsemanage/src/genhomedircon.c
> +++ b/libsemanage/src/genhomedircon.c
> @@ -308,14 +308,52 @@ done:
>         return retval;
>  }
>
> +/*
> + * Parses `file` for `key` seperated by `sep` into `out`.
> + * Returns:
> + *   true on success.
> + *   false on failure.
> + *   `out` is guaranteed to be initalised.
> + *   `fallback_set` is initalised to false, and set to true if a fallback was used.
> + */
> +static bool parse_uid_config(const char *file, const char *key, const char *sep,
> +               uid_t fallback, uid_t *out, bool *fallback_set)
> +{
> +       assert(out);
> +       assert(fallback_set);
> +
> +       *fallback_set = false;
> +
> +       char *uid_str = semanage_findval(file, key, sep);
> +       if (!uid_str || !*uid_str) {
> +               free(uid_str);
> +               *fallback_set = true;
> +               *out = fallback;
> +               return true;
> +       }
> +
> +       char *endptr;
> +       errno = 0;
> +       const unsigned long val = strtoul(uid_str, &endptr, 0);
> +
> +       if (endptr != uid_str && *endptr == '\0' && errno != ERANGE) {
> +               *out = (uid_t)val;
> +               free(uid_str);
> +               return true;
> +       }
> +
> +       free(uid_str);
> +       *fallback_set = true;
> +       *out = fallback;
> +       return false;
> +}
> +
>  static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
>  {
>         semanage_list_t *homedir_list = NULL;
>         semanage_list_t *shells = NULL;
>         fc_match_handle_t hand;
>         char *path = NULL;
> -       uid_t temp, minuid = 500, maxuid = 60000;
> -       int minuid_set = 0;
>         struct passwd *pwbuf;
>         struct stat buf;
>
> @@ -362,56 +400,25 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
>              "Conversion failed for key " key ", is its value a number?" \
>              "  Falling back to default value of `%s`.", #val);
>
> -       path = semanage_findval(PATH_ETC_LOGIN_DEFS, "UID_MIN", NULL);
> -       if (path && *path) {
> -               char *endptr;
> -               const unsigned long val = strtoul(path, &endptr, 0);
> -               if (endptr != path && *endptr == '\0') {
> -                       minuid = (uid_t)val;
> -                       minuid_set = 1;
> -               } else {
> -                       /* we were provided an invalid value, use defaults.  */
> -                       genhomedircon_warn_conv_fail("UID_MIN", FALLBACK_MINUID);
> -                       minuid = FALLBACK_MINUID;
> -                       minuid_set = 1;
> -               }
> -       }
> -       free(path);
> -       path = NULL;
> +       uid_t minuid;
> +       bool fallback_set;
> +       if (!parse_uid_config(PATH_ETC_LOGIN_DEFS, "UID_MIN", NULL, FALLBACK_MINUID, &minuid, &fallback_set))
> +               genhomedircon_warn_conv_fail("UID_MIN", FALLBACK_MINUID);
>
> -       path = semanage_findval(PATH_ETC_LOGIN_DEFS, "UID_MAX", NULL);
> -       if (path && *path) {
> -               char *endptr;
> -               const unsigned long val = strtoul(path, &endptr, 0);
> -               if (endptr != path && *endptr == '\0') {
> -                       maxuid = (uid_t)val;
> -               } else {
> -                       /* we were provided an invalid value, use defaults.  */
> -                       genhomedircon_warn_conv_fail("UID_MAX", FALLBACK_MAXUID);
> -                       maxuid = FALLBACK_MAXUID;
> -               }
> -       }
> -       free(path);
> -       path = NULL;
> +       const bool logindefs_minuid_fallback_set = fallback_set;
>
> -       path = semanage_findval(PATH_ETC_LIBUSER, "LU_UIDNUMBER", "=");
> -       if (path && *path) {
> -               char *endptr;
> -               const unsigned long val = strtoul(path, &endptr, 0);
> -               if (endptr != path && *endptr == '\0') {
> -                       temp = (uid_t)val;
> -               } else {
> -                       /* we were provided an invalid value, use defaults.  */
> -                       genhomedircon_warn_conv_fail("LU_UIDNUMBER", FALLBACK_LU_UIDNUMBER);
> -                       temp = FALLBACK_LU_UIDNUMBER;
> -               }
> -               if (!minuid_set || temp < minuid) {
> -                       minuid = temp;
> -                       minuid_set = 1;
> -               }
> -       }
> -       free(path);
> -       path = NULL;
> +       uid_t temp;
> +       if (!parse_uid_config(PATH_ETC_LIBUSER, "LU_UIDNUMBER", "=", FALLBACK_LU_UIDNUMBER, &temp, &fallback_set))
> +               genhomedircon_warn_conv_fail("LU_UIDNUMBER", FALLBACK_LU_UIDNUMBER);
> +
> +       if (logindefs_minuid_fallback_set)
> +               minuid = temp;
> +
> +       uid_t maxuid;
> +       /* We don't actually check fallback_set here, PATH_ETC_LOGIN_DEFS is the one source of
> +          truth for UID_MAX.  */
> +       if (!parse_uid_config(PATH_ETC_LOGIN_DEFS, "UID_MAX", NULL, FALLBACK_MAXUID, &maxuid, &fallback_set))
> +               genhomedircon_warn_conv_fail("UID_MAX", FALLBACK_MAXUID);
>
>  #undef genhomedircon_warn_conv_fail
>
> --
> 2.51.0
>

[PATCH v4] genhomedircon: cleanup parsing of uid config values

[Rahul Sandhu]

Parsing KV files with a separator of similar format is fairly similar,
so we may as well add a helper function to make it easier to read.

Credit to Hyland for reminding me to check for ERANGE.

Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
Reviewed-by: Hyland B. <me@ow.swag.toys>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
---
 libsemanage/src/genhomedircon.c | 107 +++++++++++++++++---------------
 1 file changed, 57 insertions(+), 50 deletions(-)

v2: rename path to something more sensible (afterall, we are parsing a
    UID!) and move the free to later, just before both return paths to
    not dereference it when checking whether we actually parsed a valid
    number or not.
v3: handle the fallback case for minuid properly such that we don't end
    up always using a fallback if minuid is not set in login.defs, and
    return a bool instead as it's a bit more sensible for what we're
    trying to return. Also, check for ERANGE.
v4: add credit to Hyland for reminding me to check for ERANGE.

diff --git a/libsemanage/src/genhomedircon.c b/libsemanage/src/genhomedircon.c
index 34056562..e91c64e6 100644
--- a/libsemanage/src/genhomedircon.c
+++ b/libsemanage/src/genhomedircon.c
@@ -308,14 +308,52 @@ done:
 	return retval;
 }
 
+/*
+ * Parses `file` for `key` seperated by `sep` into `out`.
+ * Returns:
+ *   true on success.
+ *   false on failure.
+ *   `out` is guaranteed to be initalised.
+ *   `fallback_set` is initalised to false, and set to true if a fallback was used.
+ */
+static bool parse_uid_config(const char *file, const char *key, const char *sep,
+		uid_t fallback, uid_t *out, bool *fallback_set)
+{
+	assert(out);
+	assert(fallback_set);
+
+	*fallback_set = false;
+
+	char *uid_str = semanage_findval(file, key, sep);
+	if (!uid_str || !*uid_str) {
+		free(uid_str);
+		*fallback_set = true;
+		*out = fallback;
+		return true;
+	}
+
+	char *endptr;
+	errno = 0;
+	const unsigned long val = strtoul(uid_str, &endptr, 0);
+
+	if (endptr != uid_str && *endptr == '\0' && errno != ERANGE) {
+		*out = (uid_t)val;
+		free(uid_str);
+		return true;
+	}
+
+	free(uid_str);
+	*fallback_set = true;
+	*out = fallback;
+	return false;
+}
+
 static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
 {
 	semanage_list_t *homedir_list = NULL;
 	semanage_list_t *shells = NULL;
 	fc_match_handle_t hand;
 	char *path = NULL;
-	uid_t temp, minuid = 500, maxuid = 60000;
-	int minuid_set = 0;
 	struct passwd *pwbuf;
 	struct stat buf;
 
@@ -362,56 +400,25 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
 	     "Conversion failed for key " key ", is its value a number?" \
 	     "  Falling back to default value of `%s`.", #val);
 
-	path = semanage_findval(PATH_ETC_LOGIN_DEFS, "UID_MIN", NULL);
-	if (path && *path) {
-		char *endptr;
-		const unsigned long val = strtoul(path, &endptr, 0);
-		if (endptr != path && *endptr == '\0') {
-			minuid = (uid_t)val;
-			minuid_set = 1;
-		} else {
-			/* we were provided an invalid value, use defaults.  */
-			genhomedircon_warn_conv_fail("UID_MIN", FALLBACK_MINUID);
-			minuid = FALLBACK_MINUID;
-			minuid_set = 1;
-		}
-	}
-	free(path);
-	path = NULL;
+	uid_t minuid;
+	bool fallback_set;
+	if (!parse_uid_config(PATH_ETC_LOGIN_DEFS, "UID_MIN", NULL, FALLBACK_MINUID, &minuid, &fallback_set))
+		genhomedircon_warn_conv_fail("UID_MIN", FALLBACK_MINUID);
 
-	path = semanage_findval(PATH_ETC_LOGIN_DEFS, "UID_MAX", NULL);
-	if (path && *path) {
-		char *endptr;
-		const unsigned long val = strtoul(path, &endptr, 0);
-		if (endptr != path && *endptr == '\0') {
-			maxuid = (uid_t)val;
-		} else {
-			/* we were provided an invalid value, use defaults.  */
-			genhomedircon_warn_conv_fail("UID_MAX", FALLBACK_MAXUID);
-			maxuid = FALLBACK_MAXUID;
-		}
-	}
-	free(path);
-	path = NULL;
+	const bool logindefs_minuid_fallback_set = fallback_set;
 
-	path = semanage_findval(PATH_ETC_LIBUSER, "LU_UIDNUMBER", "=");
-	if (path && *path) {
-		char *endptr;
-		const unsigned long val = strtoul(path, &endptr, 0);
-		if (endptr != path && *endptr == '\0') {
-			temp = (uid_t)val;
-		} else {
-			/* we were provided an invalid value, use defaults.  */
-			genhomedircon_warn_conv_fail("LU_UIDNUMBER", FALLBACK_LU_UIDNUMBER);
-			temp = FALLBACK_LU_UIDNUMBER;
-		}
-		if (!minuid_set || temp < minuid) {
-			minuid = temp;
-			minuid_set = 1;
-		}
-	}
-	free(path);
-	path = NULL;
+	uid_t temp;
+	if (!parse_uid_config(PATH_ETC_LIBUSER, "LU_UIDNUMBER", "=", FALLBACK_LU_UIDNUMBER, &temp, &fallback_set))
+		genhomedircon_warn_conv_fail("LU_UIDNUMBER", FALLBACK_LU_UIDNUMBER);
+
+	if (logindefs_minuid_fallback_set)
+		minuid = temp;
+
+	uid_t maxuid;
+	/* We don't actually check fallback_set here, PATH_ETC_LOGIN_DEFS is the one source of
+	   truth for UID_MAX.  */
+	if (!parse_uid_config(PATH_ETC_LOGIN_DEFS, "UID_MAX", NULL, FALLBACK_MAXUID, &maxuid, &fallback_set))
+		genhomedircon_warn_conv_fail("UID_MAX", FALLBACK_MAXUID);
 
 #undef genhomedircon_warn_conv_fail
 
-- 
2.51.0

Re: [PATCH v4] genhomedircon: cleanup parsing of uid config values

[Stephen Smalley]

On Sun, Oct 26, 2025 at 3:13 PM Rahul Sandhu <nvraxn@gmail.com> wrote:
>
> Parsing KV files with a separator of similar format is fairly similar,
> so we may as well add a helper function to make it easier to read.
>
> Credit to Hyland for reminding me to check for ERANGE.
>
> Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
> Reviewed-by: Hyland B. <me@ow.swag.toys>

I don't see any such email from Hyland, just this from you, possibly
it was bounced by the list or eaten by spam filters?

> Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> ---
>  libsemanage/src/genhomedircon.c | 107 +++++++++++++++++---------------
>  1 file changed, 57 insertions(+), 50 deletions(-)
>
> v2: rename path to something more sensible (afterall, we are parsing a
>     UID!) and move the free to later, just before both return paths to
>     not dereference it when checking whether we actually parsed a valid
>     number or not.
> v3: handle the fallback case for minuid properly such that we don't end
>     up always using a fallback if minuid is not set in login.defs, and
>     return a bool instead as it's a bit more sensible for what we're
>     trying to return. Also, check for ERANGE.
> v4: add credit to Hyland for reminding me to check for ERANGE.
>
> diff --git a/libsemanage/src/genhomedircon.c b/libsemanage/src/genhomedircon.c
> index 34056562..e91c64e6 100644
> --- a/libsemanage/src/genhomedircon.c
> +++ b/libsemanage/src/genhomedircon.c
> @@ -308,14 +308,52 @@ done:
>         return retval;
>  }
>
> +/*
> + * Parses `file` for `key` seperated by `sep` into `out`.
> + * Returns:
> + *   true on success.
> + *   false on failure.
> + *   `out` is guaranteed to be initalised.
> + *   `fallback_set` is initalised to false, and set to true if a fallback was used.
> + */
> +static bool parse_uid_config(const char *file, const char *key, const char *sep,
> +               uid_t fallback, uid_t *out, bool *fallback_set)
> +{
> +       assert(out);
> +       assert(fallback_set);
> +
> +       *fallback_set = false;
> +
> +       char *uid_str = semanage_findval(file, key, sep);
> +       if (!uid_str || !*uid_str) {
> +               free(uid_str);
> +               *fallback_set = true;
> +               *out = fallback;
> +               return true;
> +       }
> +
> +       char *endptr;
> +       errno = 0;
> +       const unsigned long val = strtoul(uid_str, &endptr, 0);
> +
> +       if (endptr != uid_str && *endptr == '\0' && errno != ERANGE) {
> +               *out = (uid_t)val;
> +               free(uid_str);
> +               return true;
> +       }
> +
> +       free(uid_str);
> +       *fallback_set = true;
> +       *out = fallback;
> +       return false;
> +}
> +
>  static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
>  {
>         semanage_list_t *homedir_list = NULL;
>         semanage_list_t *shells = NULL;
>         fc_match_handle_t hand;
>         char *path = NULL;
> -       uid_t temp, minuid = 500, maxuid = 60000;
> -       int minuid_set = 0;
>         struct passwd *pwbuf;
>         struct stat buf;
>
> @@ -362,56 +400,25 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
>              "Conversion failed for key " key ", is its value a number?" \
>              "  Falling back to default value of `%s`.", #val);
>
> -       path = semanage_findval(PATH_ETC_LOGIN_DEFS, "UID_MIN", NULL);
> -       if (path && *path) {
> -               char *endptr;
> -               const unsigned long val = strtoul(path, &endptr, 0);
> -               if (endptr != path && *endptr == '\0') {
> -                       minuid = (uid_t)val;
> -                       minuid_set = 1;
> -               } else {
> -                       /* we were provided an invalid value, use defaults.  */
> -                       genhomedircon_warn_conv_fail("UID_MIN", FALLBACK_MINUID);
> -                       minuid = FALLBACK_MINUID;
> -                       minuid_set = 1;
> -               }
> -       }
> -       free(path);
> -       path = NULL;
> +       uid_t minuid;
> +       bool fallback_set;
> +       if (!parse_uid_config(PATH_ETC_LOGIN_DEFS, "UID_MIN", NULL, FALLBACK_MINUID, &minuid, &fallback_set))
> +               genhomedircon_warn_conv_fail("UID_MIN", FALLBACK_MINUID);
>
> -       path = semanage_findval(PATH_ETC_LOGIN_DEFS, "UID_MAX", NULL);
> -       if (path && *path) {
> -               char *endptr;
> -               const unsigned long val = strtoul(path, &endptr, 0);
> -               if (endptr != path && *endptr == '\0') {
> -                       maxuid = (uid_t)val;
> -               } else {
> -                       /* we were provided an invalid value, use defaults.  */
> -                       genhomedircon_warn_conv_fail("UID_MAX", FALLBACK_MAXUID);
> -                       maxuid = FALLBACK_MAXUID;
> -               }
> -       }
> -       free(path);
> -       path = NULL;
> +       const bool logindefs_minuid_fallback_set = fallback_set;
>
> -       path = semanage_findval(PATH_ETC_LIBUSER, "LU_UIDNUMBER", "=");
> -       if (path && *path) {
> -               char *endptr;
> -               const unsigned long val = strtoul(path, &endptr, 0);
> -               if (endptr != path && *endptr == '\0') {
> -                       temp = (uid_t)val;
> -               } else {
> -                       /* we were provided an invalid value, use defaults.  */
> -                       genhomedircon_warn_conv_fail("LU_UIDNUMBER", FALLBACK_LU_UIDNUMBER);
> -                       temp = FALLBACK_LU_UIDNUMBER;
> -               }
> -               if (!minuid_set || temp < minuid) {
> -                       minuid = temp;
> -                       minuid_set = 1;
> -               }
> -       }
> -       free(path);
> -       path = NULL;
> +       uid_t temp;
> +       if (!parse_uid_config(PATH_ETC_LIBUSER, "LU_UIDNUMBER", "=", FALLBACK_LU_UIDNUMBER, &temp, &fallback_set))
> +               genhomedircon_warn_conv_fail("LU_UIDNUMBER", FALLBACK_LU_UIDNUMBER);
> +
> +       if (logindefs_minuid_fallback_set)
> +               minuid = temp;
> +
> +       uid_t maxuid;
> +       /* We don't actually check fallback_set here, PATH_ETC_LOGIN_DEFS is the one source of
> +          truth for UID_MAX.  */
> +       if (!parse_uid_config(PATH_ETC_LOGIN_DEFS, "UID_MAX", NULL, FALLBACK_MAXUID, &maxuid, &fallback_set))
> +               genhomedircon_warn_conv_fail("UID_MAX", FALLBACK_MAXUID);
>
>  #undef genhomedircon_warn_conv_fail
>
> --
> 2.51.0
>

Re: [PATCH v4] genhomedircon: cleanup parsing of uid config values

[Stephen Smalley]

On Mon, Oct 27, 2025 at 9:10 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Sun, Oct 26, 2025 at 3:13 PM Rahul Sandhu <nvraxn@gmail.com> wrote:
> >
> > Parsing KV files with a separator of similar format is fairly similar,
> > so we may as well add a helper function to make it easier to read.
> >
> > Credit to Hyland for reminding me to check for ERANGE.
> >
> > Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
> > Reviewed-by: Hyland B. <me@ow.swag.toys>
>
> I don't see any such email from Hyland, just this from you, possibly
> it was bounced by the list or eaten by spam filters?

That address bounces with no such domain, so I am not taking this as a
Reviewed-by tag.

>
> > Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> > ---
> >  libsemanage/src/genhomedircon.c | 107 +++++++++++++++++---------------
> >  1 file changed, 57 insertions(+), 50 deletions(-)
> >
> > v2: rename path to something more sensible (afterall, we are parsing a
> >     UID!) and move the free to later, just before both return paths to
> >     not dereference it when checking whether we actually parsed a valid
> >     number or not.
> > v3: handle the fallback case for minuid properly such that we don't end
> >     up always using a fallback if minuid is not set in login.defs, and
> >     return a bool instead as it's a bit more sensible for what we're
> >     trying to return. Also, check for ERANGE.
> > v4: add credit to Hyland for reminding me to check for ERANGE.
> >
> > diff --git a/libsemanage/src/genhomedircon.c b/libsemanage/src/genhomedircon.c
> > index 34056562..e91c64e6 100644
> > --- a/libsemanage/src/genhomedircon.c
> > +++ b/libsemanage/src/genhomedircon.c
> > @@ -308,14 +308,52 @@ done:
> >         return retval;
> >  }
> >
> > +/*
> > + * Parses `file` for `key` seperated by `sep` into `out`.
> > + * Returns:
> > + *   true on success.
> > + *   false on failure.
> > + *   `out` is guaranteed to be initalised.
> > + *   `fallback_set` is initalised to false, and set to true if a fallback was used.
> > + */
> > +static bool parse_uid_config(const char *file, const char *key, const char *sep,
> > +               uid_t fallback, uid_t *out, bool *fallback_set)
> > +{
> > +       assert(out);
> > +       assert(fallback_set);
> > +
> > +       *fallback_set = false;
> > +
> > +       char *uid_str = semanage_findval(file, key, sep);
> > +       if (!uid_str || !*uid_str) {
> > +               free(uid_str);
> > +               *fallback_set = true;
> > +               *out = fallback;
> > +               return true;
> > +       }
> > +
> > +       char *endptr;
> > +       errno = 0;
> > +       const unsigned long val = strtoul(uid_str, &endptr, 0);
> > +
> > +       if (endptr != uid_str && *endptr == '\0' && errno != ERANGE) {
> > +               *out = (uid_t)val;
> > +               free(uid_str);
> > +               return true;
> > +       }
> > +
> > +       free(uid_str);
> > +       *fallback_set = true;
> > +       *out = fallback;
> > +       return false;
> > +}
> > +
> >  static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
> >  {
> >         semanage_list_t *homedir_list = NULL;
> >         semanage_list_t *shells = NULL;
> >         fc_match_handle_t hand;
> >         char *path = NULL;
> > -       uid_t temp, minuid = 500, maxuid = 60000;
> > -       int minuid_set = 0;
> >         struct passwd *pwbuf;
> >         struct stat buf;
> >
> > @@ -362,56 +400,25 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
> >              "Conversion failed for key " key ", is its value a number?" \
> >              "  Falling back to default value of `%s`.", #val);
> >
> > -       path = semanage_findval(PATH_ETC_LOGIN_DEFS, "UID_MIN", NULL);
> > -       if (path && *path) {
> > -               char *endptr;
> > -               const unsigned long val = strtoul(path, &endptr, 0);
> > -               if (endptr != path && *endptr == '\0') {
> > -                       minuid = (uid_t)val;
> > -                       minuid_set = 1;
> > -               } else {
> > -                       /* we were provided an invalid value, use defaults.  */
> > -                       genhomedircon_warn_conv_fail("UID_MIN", FALLBACK_MINUID);
> > -                       minuid = FALLBACK_MINUID;
> > -                       minuid_set = 1;
> > -               }
> > -       }
> > -       free(path);
> > -       path = NULL;
> > +       uid_t minuid;
> > +       bool fallback_set;
> > +       if (!parse_uid_config(PATH_ETC_LOGIN_DEFS, "UID_MIN", NULL, FALLBACK_MINUID, &minuid, &fallback_set))
> > +               genhomedircon_warn_conv_fail("UID_MIN", FALLBACK_MINUID);
> >
> > -       path = semanage_findval(PATH_ETC_LOGIN_DEFS, "UID_MAX", NULL);
> > -       if (path && *path) {
> > -               char *endptr;
> > -               const unsigned long val = strtoul(path, &endptr, 0);
> > -               if (endptr != path && *endptr == '\0') {
> > -                       maxuid = (uid_t)val;
> > -               } else {
> > -                       /* we were provided an invalid value, use defaults.  */
> > -                       genhomedircon_warn_conv_fail("UID_MAX", FALLBACK_MAXUID);
> > -                       maxuid = FALLBACK_MAXUID;
> > -               }
> > -       }
> > -       free(path);
> > -       path = NULL;
> > +       const bool logindefs_minuid_fallback_set = fallback_set;
> >
> > -       path = semanage_findval(PATH_ETC_LIBUSER, "LU_UIDNUMBER", "=");
> > -       if (path && *path) {
> > -               char *endptr;
> > -               const unsigned long val = strtoul(path, &endptr, 0);
> > -               if (endptr != path && *endptr == '\0') {
> > -                       temp = (uid_t)val;
> > -               } else {
> > -                       /* we were provided an invalid value, use defaults.  */
> > -                       genhomedircon_warn_conv_fail("LU_UIDNUMBER", FALLBACK_LU_UIDNUMBER);
> > -                       temp = FALLBACK_LU_UIDNUMBER;
> > -               }
> > -               if (!minuid_set || temp < minuid) {
> > -                       minuid = temp;
> > -                       minuid_set = 1;
> > -               }
> > -       }
> > -       free(path);
> > -       path = NULL;
> > +       uid_t temp;
> > +       if (!parse_uid_config(PATH_ETC_LIBUSER, "LU_UIDNUMBER", "=", FALLBACK_LU_UIDNUMBER, &temp, &fallback_set))
> > +               genhomedircon_warn_conv_fail("LU_UIDNUMBER", FALLBACK_LU_UIDNUMBER);
> > +
> > +       if (logindefs_minuid_fallback_set)
> > +               minuid = temp;
> > +
> > +       uid_t maxuid;
> > +       /* We don't actually check fallback_set here, PATH_ETC_LOGIN_DEFS is the one source of
> > +          truth for UID_MAX.  */
> > +       if (!parse_uid_config(PATH_ETC_LOGIN_DEFS, "UID_MAX", NULL, FALLBACK_MAXUID, &maxuid, &fallback_set))
> > +               genhomedircon_warn_conv_fail("UID_MAX", FALLBACK_MAXUID);
> >
> >  #undef genhomedircon_warn_conv_fail
> >
> > --
> > 2.51.0
> >

Re: [PATCH v4] genhomedircon: cleanup parsing of uid config values

[Stephen Smalley]

On Sun, Oct 26, 2025 at 3:13 PM Rahul Sandhu <nvraxn@gmail.com> wrote:
>
> Parsing KV files with a separator of similar format is fairly similar,
> so we may as well add a helper function to make it easier to read.
>
> Credit to Hyland for reminding me to check for ERANGE.
>
> Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
> Reviewed-by: Hyland Belcher <hhhyland.belcherrr4@gmail.com>
> Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>

Thanks, applied with the corrected tag.

> ---
>  libsemanage/src/genhomedircon.c | 107 +++++++++++++++++---------------
>  1 file changed, 57 insertions(+), 50 deletions(-)
>
> v2: rename path to something more sensible (afterall, we are parsing a
>     UID!) and move the free to later, just before both return paths to
>     not dereference it when checking whether we actually parsed a valid
>     number or not.
> v3: handle the fallback case for minuid properly such that we don't end
>     up always using a fallback if minuid is not set in login.defs, and
>     return a bool instead as it's a bit more sensible for what we're
>     trying to return. Also, check for ERANGE.
> v4: add credit to Hyland for reminding me to check for ERANGE.
>
> diff --git a/libsemanage/src/genhomedircon.c b/libsemanage/src/genhomedircon.c
> index 34056562..e91c64e6 100644
> --- a/libsemanage/src/genhomedircon.c
> +++ b/libsemanage/src/genhomedircon.c
> @@ -308,14 +308,52 @@ done:
>         return retval;
>  }
>
> +/*
> + * Parses `file` for `key` seperated by `sep` into `out`.
> + * Returns:
> + *   true on success.
> + *   false on failure.
> + *   `out` is guaranteed to be initalised.
> + *   `fallback_set` is initalised to false, and set to true if a fallback was used.
> + */
> +static bool parse_uid_config(const char *file, const char *key, const char *sep,
> +               uid_t fallback, uid_t *out, bool *fallback_set)
> +{
> +       assert(out);
> +       assert(fallback_set);
> +
> +       *fallback_set = false;
> +
> +       char *uid_str = semanage_findval(file, key, sep);
> +       if (!uid_str || !*uid_str) {
> +               free(uid_str);
> +               *fallback_set = true;
> +               *out = fallback;
> +               return true;
> +       }
> +
> +       char *endptr;
> +       errno = 0;
> +       const unsigned long val = strtoul(uid_str, &endptr, 0);
> +
> +       if (endptr != uid_str && *endptr == '\0' && errno != ERANGE) {
> +               *out = (uid_t)val;
> +               free(uid_str);
> +               return true;
> +       }
> +
> +       free(uid_str);
> +       *fallback_set = true;
> +       *out = fallback;
> +       return false;
> +}
> +
>  static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
>  {
>         semanage_list_t *homedir_list = NULL;
>         semanage_list_t *shells = NULL;
>         fc_match_handle_t hand;
>         char *path = NULL;
> -       uid_t temp, minuid = 500, maxuid = 60000;
> -       int minuid_set = 0;
>         struct passwd *pwbuf;
>         struct stat buf;
>
> @@ -362,56 +400,25 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
>              "Conversion failed for key " key ", is its value a number?" \
>              "  Falling back to default value of `%s`.", #val);
>
> -       path = semanage_findval(PATH_ETC_LOGIN_DEFS, "UID_MIN", NULL);
> -       if (path && *path) {
> -               char *endptr;
> -               const unsigned long val = strtoul(path, &endptr, 0);
> -               if (endptr != path && *endptr == '\0') {
> -                       minuid = (uid_t)val;
> -                       minuid_set = 1;
> -               } else {
> -                       /* we were provided an invalid value, use defaults.  */
> -                       genhomedircon_warn_conv_fail("UID_MIN", FALLBACK_MINUID);
> -                       minuid = FALLBACK_MINUID;
> -                       minuid_set = 1;
> -               }
> -       }
> -       free(path);
> -       path = NULL;
> +       uid_t minuid;
> +       bool fallback_set;
> +       if (!parse_uid_config(PATH_ETC_LOGIN_DEFS, "UID_MIN", NULL, FALLBACK_MINUID, &minuid, &fallback_set))
> +               genhomedircon_warn_conv_fail("UID_MIN", FALLBACK_MINUID);
>
> -       path = semanage_findval(PATH_ETC_LOGIN_DEFS, "UID_MAX", NULL);
> -       if (path && *path) {
> -               char *endptr;
> -               const unsigned long val = strtoul(path, &endptr, 0);
> -               if (endptr != path && *endptr == '\0') {
> -                       maxuid = (uid_t)val;
> -               } else {
> -                       /* we were provided an invalid value, use defaults.  */
> -                       genhomedircon_warn_conv_fail("UID_MAX", FALLBACK_MAXUID);
> -                       maxuid = FALLBACK_MAXUID;
> -               }
> -       }
> -       free(path);
> -       path = NULL;
> +       const bool logindefs_minuid_fallback_set = fallback_set;
>
> -       path = semanage_findval(PATH_ETC_LIBUSER, "LU_UIDNUMBER", "=");
> -       if (path && *path) {
> -               char *endptr;
> -               const unsigned long val = strtoul(path, &endptr, 0);
> -               if (endptr != path && *endptr == '\0') {
> -                       temp = (uid_t)val;
> -               } else {
> -                       /* we were provided an invalid value, use defaults.  */
> -                       genhomedircon_warn_conv_fail("LU_UIDNUMBER", FALLBACK_LU_UIDNUMBER);
> -                       temp = FALLBACK_LU_UIDNUMBER;
> -               }
> -               if (!minuid_set || temp < minuid) {
> -                       minuid = temp;
> -                       minuid_set = 1;
> -               }
> -       }
> -       free(path);
> -       path = NULL;
> +       uid_t temp;
> +       if (!parse_uid_config(PATH_ETC_LIBUSER, "LU_UIDNUMBER", "=", FALLBACK_LU_UIDNUMBER, &temp, &fallback_set))
> +               genhomedircon_warn_conv_fail("LU_UIDNUMBER", FALLBACK_LU_UIDNUMBER);
> +
> +       if (logindefs_minuid_fallback_set)
> +               minuid = temp;
> +
> +       uid_t maxuid;
> +       /* We don't actually check fallback_set here, PATH_ETC_LOGIN_DEFS is the one source of
> +          truth for UID_MAX.  */
> +       if (!parse_uid_config(PATH_ETC_LOGIN_DEFS, "UID_MAX", NULL, FALLBACK_MAXUID, &maxuid, &fallback_set))
> +               genhomedircon_warn_conv_fail("UID_MAX", FALLBACK_MAXUID);
>
>  #undef genhomedircon_warn_conv_fail
>
> --
> 2.51.0
>