Eelco Chaudron <echau...@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 <echau...@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.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
