A number of fixes around `strto*()` usage #5827

alexey-tikhonov · 2021-10-15T20:35:32Z

No description provided.

alexey-tikhonov · 2021-10-15T20:37:35Z

RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1977803

elkoniu

There are few more places where strtouint*() is called with 2nd parameter set to NULL:

src/sbus/sbus_errors.c                                                          
74:        ret = strtouint32(error->message, NULL, 10);                            
                                                                                   
src/responder/kcm/kcmsrv_ccache_secdb.c                                            
846:        uid = strtouint32(uid_list[i], NULL, 10);                              
                                                                                   
src/tests/cwrap/test_server.c                                                      
79:    tmp = strtouint32(buf, NULL, 10);                                           
                                                                                   
src/providers/ldap/ldap_id_services.c                                              
266:            port = strtouint16(state->name, NULL, 10);                         
                                                                                   
src/providers/ldap/sdap_access.c                                                   
1859:            duration = strtouint32(pwdAccountLockedDurationTime, NULL, 0);

If those are outside of the scope of this PR?

elkoniu

I found call for strtoul() in one more place:

src/providers/ldap/sdap_sudo_shared.c                                           
194:            strtoul(srv_opts->max_sudo_value, &timezone, 10);               
204:        usn_number = strtoul(usn, &timezone, 10);

I think in the 2nd case evaluation against > UINT32_MAX is missing.

elkoniu · 2021-10-17T23:42:46Z

Generally LGTM, just two minor questions.

alexey-tikhonov · 2021-10-18T19:55:07Z

There are few more places where strtouint*() is called with 2nd parameter set to NULL:

I don't have very strong justification but will try my best to explain why I missed those.

src/sbus/sbus_errors.c
74: ret = strtouint32(error->message, NULL, 10);

This is a case dbus_error_has_name(error, SBUS_ERROR_ERRNO), i.e. (IIUC) error->message is set by another SSSD process, i.e. can be treated as a more or less "safe" source.
But imagine we want to be prudent and catch a case "123abc". How are we going to handle this?
This is a helper function that translates error to error code. So instead of 123 in this case of malformed error text we can return, for example, ERR_INTERNAL. Does it make some sense? Perhaps. Much sense? Imo, no.

src/responder/kcm/kcmsrv_ccache_secdb.c
846: uid = strtouint32(uid_list[i], NULL, 10);

uid_list originates from sssd-kcm ccache DB, so again, more or less reliable.

src/tests/cwrap/test_server.c
79: tmp = strtouint32(buf, NULL, 10);

That's a test. It's not that I don't care at all, but it really can be not as strict.

src/providers/ldap/ldap_id_services.c
266: port = strtouint16(state->name, NULL, 10);

Again, internal stuff. But ok, to be consistent I will add checks here...

src/providers/ldap/sdap_access.c
1859: duration = strtouint32(pwdAccountLockedDurationTime, NULL, 0);

This value is from LDAP - "pwdLockoutDuration" . I just wasn't sure what weird format it might have...
But it looks https://ldapwiki.com/wiki/PwdLockoutDuration says "Integer" so ok, I will update this as well.

alexey-tikhonov · 2021-10-18T19:58:50Z

I found call for strtoul() in one more place:
src/providers/ldap/sdap_sudo_shared.c                                           
194:            strtoul(srv_opts->max_sudo_value, &timezone, 10);               
204:        usn_number = strtoul(usn, &timezone, 10);
I think in the 2nd case evaluation against > UINT32_MAX is missing.

Why?
https://ldapwiki.com/wiki/Update%20Sequence%20Number says Update Sequence Number (USN) is a 64-bit number .

alexey-tikhonov · 2021-10-18T20:19:12Z

src/providers/ldap/ldap_id_services.c
266: port = strtouint16(state->name, NULL, 10);

Again, internal stuff. But ok, to be consistent I will add checks here...

src/providers/ldap/sdap_access.c
1859: duration = strtouint32(pwdAccountLockedDurationTime, NULL, 0);

This value is from LDAP - "pwdLockoutDuration" . I just wasn't sure what weird format it might have... But it looks https://ldapwiki.com/wiki/PwdLockoutDuration says "Integer" so ok, I will update this as well.

Done.

elkoniu

@alexey-tikhonov thank you for explanation, code vise LGTM, upstream CI is still spinning but if it will end up green ACK from me.

elkoniu · 2021-10-20T20:39:42Z

CI is green, ACK from me.

pbrezina · 2021-10-21T12:57:47Z

Hi, it would be nice to have some description in the commit message that would make clear why the changes are required/correct without the need to peek into man page.

To properly check for an error during string to number conversion one needs to: - check `errno` - check that something was really converted (i.e. start != end) - (if this is expected) check that entire string was consumed Some of those error conditions weren't checked in various locations over the code.

Previously result of `AS_STR(OFFLINE_TIMEOUT)` was "OFFLINE_TIMEOUT" instead of expected integer value.

alexey-tikhonov · 2021-10-21T21:02:58Z

Hi, it would be nice to have some description in the commit message that would make clear why the changes are required/correct without the need to peek into man page.

Done.

elkoniu

LGTM, just one small question.

elkoniu · 2021-10-21T21:14:24Z

src/providers/ldap/sdap_async_enum.c

@@ -571,9 +571,9 @@ static void enum_users_done(struct tevent_req *subreq)
        talloc_zfree(state->ctx->srv_opts->max_user_value);
        state->ctx->srv_opts->max_user_value =
                                        talloc_steal(state->ctx, usn_value);
-
+        errno = 0;


Small question: if in cases like this EOK should not be used instead of 0?

I guess errno == 0 is so common that there is no point to use EOK here explicit.

tl,dr: no, it shouldn't. See man page: the calling program should set errno to 0 before the call.

strto*() set errno in case specific errors happens, leave intact otherwise. Caller is required to reset errno before the call. This means to set it to a value that doesn't indicate any error. 0 is typically used for this purpose. Using EOK here is semantically wrong.

Thank you for detailed explanation.

pbrezina · 2021-10-25T12:31:20Z

Pushed PR: #5827

master
- a664e9c - TESTS: fixed a bug in define->string conversion
- 3c17a57 - 'strto*()': usage sanitization
- a2cc7da - 'strtonum' helpers: usage sanitization
- de6eba3 - Removed excessive includes around 'strtonum'

alexey-tikhonov added the Bugzilla label Oct 15, 2021

elkoniu reviewed Oct 17, 2021

View reviewed changes

alexey-tikhonov added the Changes requested label Oct 18, 2021

alexey-tikhonov force-pushed the bz1977803 branch from 7e74aa7 to 074bd49 Compare October 18, 2021 20:18

alexey-tikhonov added Waiting for review and removed Changes requested labels Oct 18, 2021

elkoniu reviewed Oct 18, 2021

View reviewed changes

alexey-tikhonov added 4 commits October 21, 2021 22:39

Removed excessive includes around 'strtonum'

aafba2f

TESTS: fixed a bug in define->string conversion

b9757be

Previously result of `AS_STR(OFFLINE_TIMEOUT)` was "OFFLINE_TIMEOUT" instead of expected integer value.

alexey-tikhonov force-pushed the bz1977803 branch from 074bd49 to b9757be Compare October 21, 2021 21:02

elkoniu reviewed Oct 21, 2021

View reviewed changes

elkoniu self-assigned this Oct 25, 2021

elkoniu added Accepted and removed Waiting for review labels Oct 25, 2021

pbrezina added the Ready to push Ready to push label Oct 25, 2021

pbrezina added Pushed and removed Accepted Ready to push Ready to push labels Oct 25, 2021

pbrezina closed this Oct 25, 2021

alexey-tikhonov deleted the bz1977803 branch March 31, 2022 10:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A number of fixes around `strto*()` usage #5827

A number of fixes around `strto*()` usage #5827

alexey-tikhonov commented Oct 15, 2021

alexey-tikhonov commented Oct 15, 2021

elkoniu left a comment

elkoniu left a comment

elkoniu commented Oct 17, 2021

alexey-tikhonov commented Oct 18, 2021 •

edited

Loading

alexey-tikhonov commented Oct 18, 2021

alexey-tikhonov commented Oct 18, 2021

elkoniu left a comment

elkoniu commented Oct 20, 2021

pbrezina commented Oct 21, 2021

alexey-tikhonov commented Oct 21, 2021

elkoniu left a comment

elkoniu Oct 21, 2021

elkoniu Oct 25, 2021

alexey-tikhonov Oct 25, 2021 •

edited

Loading

elkoniu Oct 25, 2021

pbrezina commented Oct 25, 2021

A number of fixes around strto*() usage #5827

A number of fixes around strto*() usage #5827

Conversation

alexey-tikhonov commented Oct 15, 2021

alexey-tikhonov commented Oct 15, 2021

elkoniu left a comment

Choose a reason for hiding this comment

elkoniu left a comment

Choose a reason for hiding this comment

elkoniu commented Oct 17, 2021

alexey-tikhonov commented Oct 18, 2021 • edited Loading

alexey-tikhonov commented Oct 18, 2021

alexey-tikhonov commented Oct 18, 2021

elkoniu left a comment

Choose a reason for hiding this comment

elkoniu commented Oct 20, 2021

pbrezina commented Oct 21, 2021

alexey-tikhonov commented Oct 21, 2021

elkoniu left a comment

Choose a reason for hiding this comment

elkoniu Oct 21, 2021

Choose a reason for hiding this comment

elkoniu Oct 25, 2021

Choose a reason for hiding this comment

alexey-tikhonov Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

elkoniu Oct 25, 2021

Choose a reason for hiding this comment

pbrezina commented Oct 25, 2021

A number of fixes around `strto*()` usage #5827

A number of fixes around `strto*()` usage #5827

alexey-tikhonov commented Oct 18, 2021 •

edited

Loading

alexey-tikhonov Oct 25, 2021 •

edited

Loading