On 17 May 2023, at 17:06, Илья Шипицин wrote:
> ср, 17 мая 2023 г. в 23:04, Kristof Provost <k...@freebsd.org>:
>
>> On 17 May 2023, at 16:58, Илья Шипицин wrote:
>>> ср, 17 мая 2023 г. в 22:43, Kristof Provost <k...@freebsd.org>:
>>>
>>>> On 17 May 2023, at 16:01, Ilya Shipitsin wrote:
>>>>> malloc was not checked against NULL, I was able
>>>>> to get core dump in case of failure
>>>>>
>>>>> Signed-off-by: Ilya Shipitsin <chipits...@gmail.com>
>>>>> ---
>>>>> src/openvpn/dco_freebsd.c | 5 +++++
>>>>> 1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c
>>>>> index 1111abeb..adbd1120 100644
>>>>> --- a/src/openvpn/dco_freebsd.c
>>>>> +++ b/src/openvpn/dco_freebsd.c
>>>>> @@ -594,6 +594,11 @@ dco_available(int msglevel)
>>>>> }
>>>>>
>>>>> buf = malloc(ifcr.ifcr_total * IFNAMSIZ);
>>>>> + if (buf == NULL)
>>>>> + {
>>>>
>>>> I’d ‘goto out;’ instead, because that’s how we handle other errors in
>> this
>>>> function.
>>>> (free(NULL) is guaranteed to be safe, so we can just do that.)
>>>>
>>>
>>> on "goto out" we'll end with "return available;"
>>>
>> ‘available’ defaults to ‘false’, so that seems fine to me.
>>
>
> looks fragile :)
>
> I do not see benefits of such an approach. But it will work indeed
>
The reasoning is that it keeps the error handing consistent. If we ever extend
the function to e.g. open another file descriptor we’d only have to change the
error handling in one location.
We’re not adding new ways for the function to fail either. The next potential
error is the ioctl() call, where we do exactly the same thing (i.e. goto out)
on error, so it already relies on available being set to false.
Best regards,
Kristof
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
