* [PATCH] libsemanage: get_home_dirs: cleanup parsing of values from conf files
@ 2025-07-30 18:15 Rahul Sandhu
2025-08-01 17:46 ` Stephen Smalley
0 siblings, 1 reply; 3+ messages in thread
From: Rahul Sandhu @ 2025-07-30 18:15 UTC (permalink / raw)
To: selinux; +Cc: Rahul Sandhu
atoi (3) is... bugprone. It's virtually impossible to differentiate an
invalid value (e.g. the string "foo") from a valid value such as "0" as
0 is returned on error! From the manual page:
> except that atoi() does not detect errors.
> RETURN VALUE
> The converted value or 0 on error.
In the case of get_home_dirs, atoi is downright wrong. We are parsing
UID_MIN, UID_MAX, and LU_UIDNUMBER, which all have a numerical value,
without any validation that what we are parsing is actually a number.
This is especially problematic as that means that in the case of an
invalid value (e.g. UID_MIN=foo), UID_MIN is incorrectly parsed as 0.
Instead, use strtoul (3) to parse these values. If parsing fails, such
as in the case where UID_MIN=foo, warn that parsing failed, and use the
default values for each key as specified by the manual page.
Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
---
libsemanage/src/genhomedircon.c | 41 ++++++++++++++++++++++++++++-----
1 file changed, 35 insertions(+), 6 deletions(-)
diff --git a/libsemanage/src/genhomedircon.c b/libsemanage/src/genhomedircon.c
index 8782e2cb..a7b44d8d 100644
--- a/libsemanage/src/genhomedircon.c
+++ b/libsemanage/src/genhomedircon.c
@@ -354,24 +354,53 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
path = semanage_findval(PATH_ETC_LOGIN_DEFS, "UID_MIN", NULL);
if (path && *path) {
- temp = atoi(path);
- minuid = temp;
- minuid_set = 1;
+ 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. */
+ WARN(s->h_semanage,
+ "Conversion failed for key UID_MIN, is its value a number?"
+ " Falling back to default value of `1000`.");
+ minuid = 1000;
+ minuid_set = 1;
+ }
}
free(path);
path = NULL;
path = semanage_findval(PATH_ETC_LOGIN_DEFS, "UID_MAX", NULL);
if (path && *path) {
- temp = atoi(path);
- maxuid = temp;
+ 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. */
+ WARN(s->h_semanage,
+ "Conversion failed for key UID_MAX, is its value a number?"
+ " Falling back to default value of `6000`.");
+ maxuid = 60000;
+ }
}
free(path);
path = NULL;
path = semanage_findval(PATH_ETC_LIBUSER, "LU_UIDNUMBER", "=");
if (path && *path) {
- temp = atoi(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. */
+ WARN(s->h_semanage,
+ "Conversion failed for key LU_UIDNUMBER, is its value a number?"
+ " Falling back to default value of `500`.");
+ temp = 500;
+ }
if (!minuid_set || temp < minuid) {
minuid = temp;
minuid_set = 1;
--
2.50.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] libsemanage: get_home_dirs: cleanup parsing of values from conf files
2025-07-30 18:15 [PATCH] libsemanage: get_home_dirs: cleanup parsing of values from conf files Rahul Sandhu
@ 2025-08-01 17:46 ` Stephen Smalley
2025-08-07 6:07 ` Fei Shao
0 siblings, 1 reply; 3+ messages in thread
From: Stephen Smalley @ 2025-08-01 17:46 UTC (permalink / raw)
To: Rahul Sandhu; +Cc: selinux
On Wed, Jul 30, 2025 at 2:15 PM Rahul Sandhu <nvraxn@gmail.com> wrote:
>
> atoi (3) is... bugprone. It's virtually impossible to differentiate an
> invalid value (e.g. the string "foo") from a valid value such as "0" as
> 0 is returned on error! From the manual page:
>
> > except that atoi() does not detect errors.
> > RETURN VALUE
> > The converted value or 0 on error.
>
> In the case of get_home_dirs, atoi is downright wrong. We are parsing
> UID_MIN, UID_MAX, and LU_UIDNUMBER, which all have a numerical value,
> without any validation that what we are parsing is actually a number.
> This is especially problematic as that means that in the case of an
> invalid value (e.g. UID_MIN=foo), UID_MIN is incorrectly parsed as 0.
>
> Instead, use strtoul (3) to parse these values. If parsing fails, such
> as in the case where UID_MIN=foo, warn that parsing failed, and use the
> default values for each key as specified by the manual page.
>
> Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
> ---
> libsemanage/src/genhomedircon.c | 41 ++++++++++++++++++++++++++++-----
> 1 file changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/libsemanage/src/genhomedircon.c b/libsemanage/src/genhomedircon.c
> index 8782e2cb..a7b44d8d 100644
> --- a/libsemanage/src/genhomedircon.c
> +++ b/libsemanage/src/genhomedircon.c
> @@ -354,24 +354,53 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
>
> path = semanage_findval(PATH_ETC_LOGIN_DEFS, "UID_MIN", NULL);
> if (path && *path) {
> - temp = atoi(path);
> - minuid = temp;
> - minuid_set = 1;
> + 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. */
> + WARN(s->h_semanage,
> + "Conversion failed for key UID_MIN, is its value a number?"
> + " Falling back to default value of `1000`.");
> + minuid = 1000;
Here and below, it would be nice if we could use a #define, either a
pre-existing one or one of our own, and avoid manual duplication of
the value/string.
> + minuid_set = 1;
> + }
> }
> free(path);
> path = NULL;
>
> path = semanage_findval(PATH_ETC_LOGIN_DEFS, "UID_MAX", NULL);
> if (path && *path) {
> - temp = atoi(path);
> - maxuid = temp;
> + 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. */
> + WARN(s->h_semanage,
> + "Conversion failed for key UID_MAX, is its value a number?"
> + " Falling back to default value of `6000`.");
Note the inconsistency here, which would be avoided by the approach
suggested above.
> + maxuid = 60000;
> + }
> }
> free(path);
> path = NULL;
>
> path = semanage_findval(PATH_ETC_LIBUSER, "LU_UIDNUMBER", "=");
> if (path && *path) {
> - temp = atoi(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. */
> + WARN(s->h_semanage,
> + "Conversion failed for key LU_UIDNUMBER, is its value a number?"
> + " Falling back to default value of `500`.");
> + temp = 500;
Ditto.
> + }
> if (!minuid_set || temp < minuid) {
> minuid = temp;
> minuid_set = 1;
> --
> 2.50.1
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] libsemanage: get_home_dirs: cleanup parsing of values from conf files
2025-08-01 17:46 ` Stephen Smalley
@ 2025-08-07 6:07 ` Fei Shao
0 siblings, 0 replies; 3+ messages in thread
From: Fei Shao @ 2025-08-07 6:07 UTC (permalink / raw)
To: Stephen Smalley, Rahul Sandhu; +Cc: selinux
on 2025-08-02 1:46, Stephen Smalley wrote:
> On Wed, Jul 30, 2025 at 2:15 PM Rahul Sandhu <nvraxn@gmail.com> wrote:
>> atoi (3) is... bugprone. It's virtually impossible to differentiate an
>> invalid value (e.g. the string "foo") from a valid value such as "0" as
>> 0 is returned on error! From the manual page:
>>
>>> except that atoi() does not detect errors.
>>> RETURN VALUE
>>> The converted value or 0 on error.
>> In the case of get_home_dirs, atoi is downright wrong. We are parsing
>> UID_MIN, UID_MAX, and LU_UIDNUMBER, which all have a numerical value,
>> without any validation that what we are parsing is actually a number.
>> This is especially problematic as that means that in the case of an
>> invalid value (e.g. UID_MIN=foo), UID_MIN is incorrectly parsed as 0.
>>
>> Instead, use strtoul (3) to parse these values. If parsing fails, such
>> as in the case where UID_MIN=foo, warn that parsing failed, and use the
>> default values for each key as specified by the manual page.
>>
>> Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
>> ---
>> libsemanage/src/genhomedircon.c | 41 ++++++++++++++++++++++++++++-----
>> 1 file changed, 35 insertions(+), 6 deletions(-)
>>
>> diff --git a/libsemanage/src/genhomedircon.c b/libsemanage/src/genhomedircon.c
>> index 8782e2cb..a7b44d8d 100644
>> --- a/libsemanage/src/genhomedircon.c
>> +++ b/libsemanage/src/genhomedircon.c
>> @@ -354,24 +354,53 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
>>
>> path = semanage_findval(PATH_ETC_LOGIN_DEFS, "UID_MIN", NULL);
>> if (path && *path) {
>> - temp = atoi(path);
>> - minuid = temp;
>> - minuid_set = 1;
>> + 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. */
>> + WARN(s->h_semanage,
>> + "Conversion failed for key UID_MIN, is its value a number?"
>> + " Falling back to default value of `1000`.");
>> + minuid = 1000;
> Here and below, it would be nice if we could use a #define, either a
> pre-existing one or one of our own, and avoid manual duplication of
> the value/string.
At top of get_home_dirs, the minuid is set to 500 default, Should the
value be reset to 500 by default here?
>> + minuid_set = 1;
In both the if and else branches,|minuid_set|is set to 1 — could this be
moved outside the conditional blocks and implemented with just a single
line of code?
>> + }
>> }
>> free(path);
>> path = NULL;
>>
>> path = semanage_findval(PATH_ETC_LOGIN_DEFS, "UID_MAX", NULL);
>> if (path && *path) {
>> - temp = atoi(path);
>> - maxuid = temp;
>> + 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. */
>> + WARN(s->h_semanage,
>> + "Conversion failed for key UID_MAX, is its value a number?"
>> + " Falling back to default value of `6000`.");
6000 or 60000?
> Note the inconsistency here, which would be avoided by the approach
> suggested above.
>
>> + maxuid = 60000;
>> + }
>> }
>> free(path);
>> path = NULL;
>>
>> path = semanage_findval(PATH_ETC_LIBUSER, "LU_UIDNUMBER", "=");
>> if (path && *path) {
>> - temp = atoi(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. */
>> + WARN(s->h_semanage,
>> + "Conversion failed for key LU_UIDNUMBER, is its value a number?"
>> + " Falling back to default value of `500`.");
>> + temp = 500;
> Ditto.
>
>> + }
>> if (!minuid_set || temp < minuid) {
>> minuid = temp;
>> minuid_set = 1;
>> --
>> 2.50.1
>>
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-08-07 6:07 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-30 18:15 [PATCH] libsemanage: get_home_dirs: cleanup parsing of values from conf files Rahul Sandhu
2025-08-01 17:46 ` Stephen Smalley
2025-08-07 6:07 ` Fei Shao
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).