| Message ID | 65dff79f725a94bb7d7f958533f993d644744302.1749133911.git.echaudro@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | aaron conole |
| Headers | show |
| Series | Various Coverity fixes. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/cirrus-robot | success | cirrus build: passed |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
Eelco Chaudron via dev <ovs-dev@openvswitch.org> writes: > Coverity reports that daemon_set_new_user() may receive a large > unsigned value from get_sysconf_buffer_size(), due to sysconf() > returning -1 and being cast to size_t. That's weird that it's complaining about the upcast. I might be misremembering the casting rules, though. I did check the latest c-standard (c22) and it did have this to say in 6.3.1.8 about conversions in comparisons:: Otherwise, if the operand that has unsigned integer type has rank greater or equal to the rank of the type of the other operand, then the operand with signed integer type is converted to the type of the operand with unsigned integer type. That verbiage was adopted with c11, IIRC - and I think it means that the compiler would implicitly convert the types correctly (rather than the older conversion rules that would have resulted in the -1 being UINT_MAX, again assuming I'm reading it correctly). > Although this would likely lead to an allocation failure and abort, > it's better to handle the error in place. > > Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > --- > lib/daemon-unix.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c > index 4fdc6e3c4..78a013645 100644 > --- a/lib/daemon-unix.c > +++ b/lib/daemon-unix.c > @@ -915,8 +915,9 @@ daemon_become_new_user(bool access_datapath, bool access_hardware_ports) > static size_t > get_sysconf_buffer_size(void) > { > - size_t bufsize, pwd_bs = 0, grp_bs = 0; > const size_t default_bufsize = 1024; > + long pwd_bs = 0, grp_bs = 0; > + size_t bufsize; > > errno = 0; > if ((pwd_bs = sysconf(_SC_GETPW_R_SIZE_MAX)) == -1) { > @@ -924,14 +925,19 @@ get_sysconf_buffer_size(void) > VLOG_FATAL("%s: Read initial passwordd struct size " > "failed (%s), aborting. ", pidfile, > ovs_strerror(errno)); > + } else { > + pwd_bs = default_bufsize; > } > } > > + errno = 0; > if ((grp_bs = sysconf(_SC_GETGR_R_SIZE_MAX)) == -1) { > if (errno) { > VLOG_FATAL("%s: Read initial group struct size " > "failed (%s), aborting. ", pidfile, > ovs_strerror(errno)); > + } else { > + grp_bs = default_bufsize; > } > } Won't the snippet at the bottom handle the else cases here: bufsize = MAX(pwd_bs, grp_bs); return bufsize ? bufsize : default_bufsize; The rest of the code that uses get_pwname_r / get_pwuid_r also will loop through correctly if the buffersize is unsufficient. If I understand correcly, could this all be avoided by casting the -1 to size_t in the 'if' conditions?
On 5 Jun 2025, at 19:21, Aaron Conole wrote: > Eelco Chaudron via dev <ovs-dev@openvswitch.org> writes: > >> Coverity reports that daemon_set_new_user() may receive a large >> unsigned value from get_sysconf_buffer_size(), due to sysconf() >> returning -1 and being cast to size_t. > > That's weird that it's complaining about the upcast. I might be > misremembering the casting rules, though. I did check the latest > c-standard (c22) and it did have this to say in 6.3.1.8 about > conversions in comparisons:: > > Otherwise, if the operand that has unsigned integer type has rank > greater or equal to the rank of the type of the other operand, then > the operand with signed integer type is converted to the type of the > operand with unsigned integer type. > > That verbiage was adopted with c11, IIRC - and I think it means that the > compiler would implicitly convert the types correctly (rather than the > older conversion rules that would have resulted in the -1 being > UINT_MAX, again assuming I'm reading it correctly). Yes, I was (am) confused also, just to make sure it clear, I converted the result code to long as per API and it will be clear what we are trying to do here. >> Although this would likely lead to an allocation failure and abort, >> it's better to handle the error in place. >> >> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >> --- >> lib/daemon-unix.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c >> index 4fdc6e3c4..78a013645 100644 >> --- a/lib/daemon-unix.c >> +++ b/lib/daemon-unix.c >> @@ -915,8 +915,9 @@ daemon_become_new_user(bool access_datapath, bool access_hardware_ports) >> static size_t >> get_sysconf_buffer_size(void) >> { >> - size_t bufsize, pwd_bs = 0, grp_bs = 0; >> const size_t default_bufsize = 1024; >> + long pwd_bs = 0, grp_bs = 0; >> + size_t bufsize; >> >> errno = 0; >> if ((pwd_bs = sysconf(_SC_GETPW_R_SIZE_MAX)) == -1) { >> @@ -924,14 +925,19 @@ get_sysconf_buffer_size(void) >> VLOG_FATAL("%s: Read initial passwordd struct size " >> "failed (%s), aborting. ", pidfile, >> ovs_strerror(errno)); >> + } else { >> + pwd_bs = default_bufsize; >> } >> } >> >> + errno = 0; >> if ((grp_bs = sysconf(_SC_GETGR_R_SIZE_MAX)) == -1) { >> if (errno) { >> VLOG_FATAL("%s: Read initial group struct size " >> "failed (%s), aborting. ", pidfile, >> ovs_strerror(errno)); >> + } else { >> + grp_bs = default_bufsize; >> } >> } > > Won't the snippet at the bottom handle the else cases here: > > bufsize = MAX(pwd_bs, grp_bs); > return bufsize ? bufsize : default_bufsize; No, it does not, as -1 can be returned without an error, meaning the value is not defined. So in that case, it will be set to SIZE_MAX. > The rest of the code that uses get_pwname_r / get_pwuid_r also will loop > through correctly if the buffersize is unsufficient. > > If I understand correcly, could this all be avoided by casting the -1 to > size_t in the 'if' conditions? Not sure what you mean, this; if ((pwd_bs = sysconf(_SC_GETPW_R_SIZE_MAX)) == (size_t) -1) { This will not take care of the case when it returns -1 without an error.
Eelco Chaudron <echaudro@redhat.com> writes: > On 5 Jun 2025, at 19:21, Aaron Conole wrote: > >> Eelco Chaudron via dev <ovs-dev@openvswitch.org> writes: >> >>> Coverity reports that daemon_set_new_user() may receive a large >>> unsigned value from get_sysconf_buffer_size(), due to sysconf() >>> returning -1 and being cast to size_t. >> >> That's weird that it's complaining about the upcast. I might be >> misremembering the casting rules, though. I did check the latest >> c-standard (c22) and it did have this to say in 6.3.1.8 about >> conversions in comparisons:: >> >> Otherwise, if the operand that has unsigned integer type has rank >> greater or equal to the rank of the type of the other operand, then >> the operand with signed integer type is converted to the type of the >> operand with unsigned integer type. >> >> That verbiage was adopted with c11, IIRC - and I think it means that the >> compiler would implicitly convert the types correctly (rather than the >> older conversion rules that would have resulted in the -1 being >> UINT_MAX, again assuming I'm reading it correctly). > > Yes, I was (am) confused also, just to make sure it clear, I converted > the result code to long as per API and it will be clear what we are > trying to do here. Okay, as long as we're both confused equally :) >>> Although this would likely lead to an allocation failure and abort, >>> it's better to handle the error in place. >>> >>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >>> --- >>> lib/daemon-unix.c | 8 +++++++- >>> 1 file changed, 7 insertions(+), 1 deletion(-) >>> >>> diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c >>> index 4fdc6e3c4..78a013645 100644 >>> --- a/lib/daemon-unix.c >>> +++ b/lib/daemon-unix.c >>> @@ -915,8 +915,9 @@ daemon_become_new_user(bool access_datapath, bool access_hardware_ports) >>> static size_t >>> get_sysconf_buffer_size(void) >>> { >>> - size_t bufsize, pwd_bs = 0, grp_bs = 0; >>> const size_t default_bufsize = 1024; >>> + long pwd_bs = 0, grp_bs = 0; >>> + size_t bufsize; >>> >>> errno = 0; >>> if ((pwd_bs = sysconf(_SC_GETPW_R_SIZE_MAX)) == -1) { >>> @@ -924,14 +925,19 @@ get_sysconf_buffer_size(void) >>> VLOG_FATAL("%s: Read initial passwordd struct size " >>> "failed (%s), aborting. ", pidfile, >>> ovs_strerror(errno)); >>> + } else { >>> + pwd_bs = default_bufsize; >>> } >>> } >>> >>> + errno = 0; >>> if ((grp_bs = sysconf(_SC_GETGR_R_SIZE_MAX)) == -1) { >>> if (errno) { >>> VLOG_FATAL("%s: Read initial group struct size " >>> "failed (%s), aborting. ", pidfile, >>> ovs_strerror(errno)); >>> + } else { >>> + grp_bs = default_bufsize; >>> } >>> } >> >> Won't the snippet at the bottom handle the else cases here: >> >> bufsize = MAX(pwd_bs, grp_bs); >> return bufsize ? bufsize : default_bufsize; > > No, it does not, as -1 can be returned without an error, meaning the > value is not defined. So in that case, it will be set to SIZE_MAX. Right, as The Open Group Base Specifications Issue 7, 2018 edition says:: A call to sysconf(_SC_GETPW_R_SIZE_MAX) returns either -1 without changing errno or an initial value suggested for the size of this buffer. ... Note that sysconf(_SC_GETPW_R_SIZE_MAX) may return -1 if there is no hard limit on the size of the buffer needed to store all the groups returned. And in that case, we will want the setting to be correctly set to default buffer size. >> The rest of the code that uses get_pwname_r / get_pwuid_r also will loop >> through correctly if the buffersize is unsufficient. >> >> If I understand correcly, could this all be avoided by casting the -1 to >> size_t in the 'if' conditions? > > Not sure what you mean, this; > > if ((pwd_bs = sysconf(_SC_GETPW_R_SIZE_MAX)) == (size_t) -1) { > > This will not take care of the case when it returns -1 without an error. Agreed. At this point, I don't want to paint the shed any specific color, so this looks okay to me. I think it may be possible to still handle the error condition using some combination of MAX/MIN, but it probably is okay. That said, it may still be useful to document in the `if` blocks that 'sysconf(_SC_GETPW_R_SIZE_MAX)', etc. can return -1 with no error if they don't recommend a specific size. That way it is also clearer on review without needing to look it up.
On 6 Jun 2025, at 16:06, Aaron Conole wrote: > Eelco Chaudron <echaudro@redhat.com> writes: > >> On 5 Jun 2025, at 19:21, Aaron Conole wrote: >> >>> Eelco Chaudron via dev <ovs-dev@openvswitch.org> writes: >>> >>>> Coverity reports that daemon_set_new_user() may receive a large >>>> unsigned value from get_sysconf_buffer_size(), due to sysconf() >>>> returning -1 and being cast to size_t. >>> >>> That's weird that it's complaining about the upcast. I might be >>> misremembering the casting rules, though. I did check the latest >>> c-standard (c22) and it did have this to say in 6.3.1.8 about >>> conversions in comparisons:: >>> >>> Otherwise, if the operand that has unsigned integer type has rank >>> greater or equal to the rank of the type of the other operand, then >>> the operand with signed integer type is converted to the type of the >>> operand with unsigned integer type. >>> >>> That verbiage was adopted with c11, IIRC - and I think it means that the >>> compiler would implicitly convert the types correctly (rather than the >>> older conversion rules that would have resulted in the -1 being >>> UINT_MAX, again assuming I'm reading it correctly). >> >> Yes, I was (am) confused also, just to make sure it clear, I converted >> the result code to long as per API and it will be clear what we are >> trying to do here. > > Okay, as long as we're both confused equally :) > >>>> Although this would likely lead to an allocation failure and abort, >>>> it's better to handle the error in place. >>>> >>>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >>>> --- >>>> lib/daemon-unix.c | 8 +++++++- >>>> 1 file changed, 7 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c >>>> index 4fdc6e3c4..78a013645 100644 >>>> --- a/lib/daemon-unix.c >>>> +++ b/lib/daemon-unix.c >>>> @@ -915,8 +915,9 @@ daemon_become_new_user(bool access_datapath, bool access_hardware_ports) >>>> static size_t >>>> get_sysconf_buffer_size(void) >>>> { >>>> - size_t bufsize, pwd_bs = 0, grp_bs = 0; >>>> const size_t default_bufsize = 1024; >>>> + long pwd_bs = 0, grp_bs = 0; >>>> + size_t bufsize; >>>> >>>> errno = 0; >>>> if ((pwd_bs = sysconf(_SC_GETPW_R_SIZE_MAX)) == -1) { >>>> @@ -924,14 +925,19 @@ get_sysconf_buffer_size(void) >>>> VLOG_FATAL("%s: Read initial passwordd struct size " >>>> "failed (%s), aborting. ", pidfile, >>>> ovs_strerror(errno)); >>>> + } else { >>>> + pwd_bs = default_bufsize; >>>> } >>>> } >>>> >>>> + errno = 0; >>>> if ((grp_bs = sysconf(_SC_GETGR_R_SIZE_MAX)) == -1) { >>>> if (errno) { >>>> VLOG_FATAL("%s: Read initial group struct size " >>>> "failed (%s), aborting. ", pidfile, >>>> ovs_strerror(errno)); >>>> + } else { >>>> + grp_bs = default_bufsize; >>>> } >>>> } >>> >>> Won't the snippet at the bottom handle the else cases here: >>> >>> bufsize = MAX(pwd_bs, grp_bs); >>> return bufsize ? bufsize : default_bufsize; >> >> No, it does not, as -1 can be returned without an error, meaning the >> value is not defined. So in that case, it will be set to SIZE_MAX. > > Right, as The Open Group Base Specifications Issue 7, 2018 edition > says:: > > A call to sysconf(_SC_GETPW_R_SIZE_MAX) returns either -1 without > changing errno or an initial value suggested for the size of this > buffer. > > ... > > Note that sysconf(_SC_GETPW_R_SIZE_MAX) may return -1 if there is no > hard limit on the size of the buffer needed to store all the groups > returned. > > And in that case, we will want the setting to be correctly set to > default buffer size. > >>> The rest of the code that uses get_pwname_r / get_pwuid_r also will loop >>> through correctly if the buffersize is unsufficient. >>> >>> If I understand correcly, could this all be avoided by casting the -1 to >>> size_t in the 'if' conditions? >> >> Not sure what you mean, this; >> >> if ((pwd_bs = sysconf(_SC_GETPW_R_SIZE_MAX)) == (size_t) -1) { >> >> This will not take care of the case when it returns -1 without an error. > > Agreed. At this point, I don't want to paint the shed any specific > color, so this looks okay to me. I think it may be possible to still > handle the error condition using some combination of MAX/MIN, but it > probably is okay. > > That said, it may still be useful to document in the `if` blocks that > 'sysconf(_SC_GETPW_R_SIZE_MAX)', etc. can return -1 with no error if > they don't recommend a specific size. That way it is also clearer on > review without needing to look it up. Thanks for the review comments Aaron! I applied the first 6 patches and sent a new revision for this patch, including some comments. Cheers, Eelco
diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c index 4fdc6e3c4..78a013645 100644 --- a/lib/daemon-unix.c +++ b/lib/daemon-unix.c @@ -915,8 +915,9 @@ daemon_become_new_user(bool access_datapath, bool access_hardware_ports) static size_t get_sysconf_buffer_size(void) { - size_t bufsize, pwd_bs = 0, grp_bs = 0; const size_t default_bufsize = 1024; + long pwd_bs = 0, grp_bs = 0; + size_t bufsize; errno = 0; if ((pwd_bs = sysconf(_SC_GETPW_R_SIZE_MAX)) == -1) { @@ -924,14 +925,19 @@ get_sysconf_buffer_size(void) VLOG_FATAL("%s: Read initial passwordd struct size " "failed (%s), aborting. ", pidfile, ovs_strerror(errno)); + } else { + pwd_bs = default_bufsize; } } + errno = 0; if ((grp_bs = sysconf(_SC_GETGR_R_SIZE_MAX)) == -1) { if (errno) { VLOG_FATAL("%s: Read initial group struct size " "failed (%s), aborting. ", pidfile, ovs_strerror(errno)); + } else { + grp_bs = default_bufsize; } }
Coverity reports that daemon_set_new_user() may receive a large unsigned value from get_sysconf_buffer_size(), due to sysconf() returning -1 and being cast to size_t. Although this would likely lead to an allocation failure and abort, it's better to handle the error in place. Signed-off-by: Eelco Chaudron <echaudro@redhat.com> --- lib/daemon-unix.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)