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