On 6 Jun 2025, at 16:06, Aaron Conole wrote:
> 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.
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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
