Re: [Openvpn-devel] [PATCH] src/openvpn/dco_freebsd.c: handle malloc failure
Am 18.05.23 um 09:09 schrieb Gert Doering: Hi, On Thu, May 18, 2023 at 09:00:26AM +0200, Matthias Andree wrote: That, and constants usually go on the left-hand side of comparison so the compiler flags the accidental if (foo = NULL) even if it does not produce "add a pair of parentheses if you really mean to use an assignment as if() expression" like warnings. You can't if (NULL = foo)... This is actually not OpenVPN style today (it's "syzzer style" so we find it in a few crypto modules). I personally find if (B) or if (!B) more concise and clearer, too. ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] src/openvpn/dco_freebsd.c: handle malloc failure
чт, 18 мая 2023 г. в 09:10, Илья Шипицин :
>
>
> чт, 18 мая 2023 г. в 08:47, Gert Doering :
>
>> Hi,
>>
>> On Wed, May 17, 2023 at 04:43:01PM -0400, Kristof Provost wrote:
>> > Fwiw: I usually don???t bother handling malloc failure in userspace,
>> > because modern systems all overallocate anyway, so the first thing
>> > you know about lack of memory is the out-of-memory killer terminating
>> > you. It???s a policy choice for the project, so I don???t object
>> > to handling it either.
>>
>> In OpenVPN, we do check malloc() returns :-) - and there's cases where
>> it actually hit, like running with too low ulimits and --mlock.
>>
>> If a malloc fail is not recoverable, we have check_malloc_return(p),
>> which will do
>>
>> void
>> out_of_memory(void)
>> {
>> fprintf(stderr, PACKAGE_NAME ": Out of Memory\n");
>> exit(1);
>> }
>> static inline void
>> check_malloc_return(const void *p)
>> {
>> if (!p)
>> {
>> out_of_memory();
>> }
>> }
>>
>> so it depends a bit on the context if "return an error upstream, and
>> wait for the next allocation to fail" or "abort right away" is the best
>> choice. In low level functions, "return error" might lead to a better
>> error message from upstream (or not).
>>
>
> this one is recoverable.
> if malloc fails, dco fails, but openvpn still able to function
>
maybe we should add something like "data channel offload failed due to ..."
>
>
>>
>> gert
>> --
>> "If was one thing all people took for granted, was conviction that if you
>> feed honest figures into a computer, honest figures come out. Never
>> doubted
>> it myself till I met a computer with a sense of humor."
>> Robert A. Heinlein, The Moon is a Harsh
>> Mistress
>>
>> Gert Doering - Munich, Germany
>> g...@greenie.muc.de
>>
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] src/openvpn/dco_freebsd.c: handle malloc failure
чт, 18 мая 2023 г. в 08:47, Gert Doering :
> Hi,
>
> On Wed, May 17, 2023 at 04:43:01PM -0400, Kristof Provost wrote:
> > Fwiw: I usually don???t bother handling malloc failure in userspace,
> > because modern systems all overallocate anyway, so the first thing
> > you know about lack of memory is the out-of-memory killer terminating
> > you. It???s a policy choice for the project, so I don???t object
> > to handling it either.
>
> In OpenVPN, we do check malloc() returns :-) - and there's cases where
> it actually hit, like running with too low ulimits and --mlock.
>
> If a malloc fail is not recoverable, we have check_malloc_return(p),
> which will do
>
> void
> out_of_memory(void)
> {
> fprintf(stderr, PACKAGE_NAME ": Out of Memory\n");
> exit(1);
> }
> static inline void
> check_malloc_return(const void *p)
> {
> if (!p)
> {
> out_of_memory();
> }
> }
>
> so it depends a bit on the context if "return an error upstream, and
> wait for the next allocation to fail" or "abort right away" is the best
> choice. In low level functions, "return error" might lead to a better
> error message from upstream (or not).
>
this one is recoverable.
if malloc fails, dco fails, but openvpn still able to function
>
> gert
> --
> "If was one thing all people took for granted, was conviction that if you
> feed honest figures into a computer, honest figures come out. Never
> doubted
> it myself till I met a computer with a sense of humor."
> Robert A. Heinlein, The Moon is a Harsh
> Mistress
>
> Gert Doering - Munich, Germany
> g...@greenie.muc.de
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] src/openvpn/dco_freebsd.c: handle malloc failure
Hi, On Thu, May 18, 2023 at 09:00:26AM +0200, Matthias Andree wrote: > That, and constants usually go on the left-hand side of comparison so > the compiler flags the accidental if (foo = NULL) even if it does not > produce "add a pair of parentheses if you really mean to use an > assignment as if() expression" like warnings. You can't if (NULL = foo)... This is actually not OpenVPN style today (it's "syzzer style" so we find it in a few crypto modules). gert -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress Gert Doering - Munich, Germany g...@greenie.muc.de signature.asc Description: PGP signature ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] src/openvpn/dco_freebsd.c: handle malloc failure
Am 17.05.23 um 22:47 schrieb Antonio Quartulli: Hi, On 17/05/2023 22: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 --- 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 abeb..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) as style guideline, we wanted to go for if (!A) rather than if (A == NULL). Although I am not sure if the whole codebase was cleaned up yet or not. That, and constants usually go on the left-hand side of comparison so the compiler flags the accidental if (foo = NULL) even if it does not produce "add a pair of parentheses if you really mean to use an assignment as if() expression" like warnings. You can't if (NULL = foo)... And in practice, if anyone hits C++ some day, it might make a difference if you're in boolean or pointer contexts, so use if (!A). ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] src/openvpn/dco_freebsd.c: handle malloc failure
Hi,
On Wed, May 17, 2023 at 04:43:01PM -0400, Kristof Provost wrote:
> Fwiw: I usually don???t bother handling malloc failure in userspace,
> because modern systems all overallocate anyway, so the first thing
> you know about lack of memory is the out-of-memory killer terminating
> you. It???s a policy choice for the project, so I don???t object
> to handling it either.
In OpenVPN, we do check malloc() returns :-) - and there's cases where
it actually hit, like running with too low ulimits and --mlock.
If a malloc fail is not recoverable, we have check_malloc_return(p),
which will do
void
out_of_memory(void)
{
fprintf(stderr, PACKAGE_NAME ": Out of Memory\n");
exit(1);
}
static inline void
check_malloc_return(const void *p)
{
if (!p)
{
out_of_memory();
}
}
so it depends a bit on the context if "return an error upstream, and
wait for the next allocation to fail" or "abort right away" is the best
choice. In low level functions, "return error" might lead to a better
error message from upstream (or not).
gert
--
"If was one thing all people took for granted, was conviction that if you
feed honest figures into a computer, honest figures come out. Never doubted
it myself till I met a computer with a sense of humor."
Robert A. Heinlein, The Moon is a Harsh Mistress
Gert Doering - Munich, Germany g...@greenie.muc.de
signature.asc
Description: PGP signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] src/openvpn/dco_freebsd.c: handle malloc failure
Hi,
On 17/05/2023 23:10, Kristof Provost wrote:
On 17 May 2023, at 17:06, Илья Шипицин wrote:
ср, 17 мая 2023 г. в 23:04, Kristof Provost :
On 17 May 2023, at 16:58, Илья Шипицин wrote:
ср, 17 мая 2023 г. в 22:43, Kristof Provost :
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
---
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 abeb..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.
I agree on this style too.
Should we require more clean up work in the future, we can just add
extra lines right after the out label (with the appropriate checks, of
course), thus keeping error handling in one place.
Cheers,
Best regards,
Kristof
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
--
Antonio Quartulli
--
Antonio Quartulli
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] src/openvpn/dco_freebsd.c: handle malloc failure
On 17 May 2023, at 17:06, Илья Шипицин wrote:
> ср, 17 мая 2023 г. в 23:04, Kristof Provost :
>
>> On 17 May 2023, at 16:58, Илья Шипицин wrote:
>>> ср, 17 мая 2023 г. в 22:43, Kristof Provost :
>>>
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
> ---
> 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 abeb..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
Re: [Openvpn-devel] [PATCH] src/openvpn/dco_freebsd.c: handle malloc failure
ср, 17 мая 2023 г. в 23:04, Kristof Provost :
> On 17 May 2023, at 16:58, Илья Шипицин wrote:
> > ср, 17 мая 2023 г. в 22:43, Kristof Provost :
> >
> >> 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
> >>> ---
> >>> 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 abeb..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
>
> Best regards,
> Kristof
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] src/openvpn/dco_freebsd.c: handle malloc failure
On 17 May 2023, at 16:58, Илья Шипицин wrote:
> ср, 17 мая 2023 г. в 22:43, Kristof Provost :
>
>> 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
>>> ---
>>> 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 abeb..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.
Best regards,
Kristof
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] src/openvpn/dco_freebsd.c: handle malloc failure
ср, 17 мая 2023 г. в 22:47, Antonio Quartulli :
> Hi,
>
> On 17/05/2023 22: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
> > ---
> > 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 abeb..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)
>
> as style guideline, we wanted to go for if (!A) rather than if (A ==
> NULL). Although I am not sure if the whole codebase was cleaned up yet
> or not.
>
I'm fine with either :)
>
> Cheers,
>
> > +{
> > +close(fd);
> > +return false;
> > +}
> >
> > ifcr.ifcr_count = ifcr.ifcr_total;
> > ifcr.ifcr_buffer = buf;
>
> --
> Antonio Quartulli
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] src/openvpn/dco_freebsd.c: handle malloc failure
ср, 17 мая 2023 г. в 22:43, Kristof Provost :
> 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
> > ---
> > 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 abeb..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;"
>
> Fwiw: I usually don’t bother handling malloc failure in userspace, because
> modern systems all overallocate anyway, so the first thing you know about
> lack of memory is the out-of-memory killer terminating you. It’s a policy
> choice for the project, so I don’t object to handling it either.
>
I agree it's a highly unlikely condition.
>
> Best regards,
> Kristof
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] src/openvpn/dco_freebsd.c: handle malloc failure
Hi,
On 17/05/2023 22: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
---
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 abeb..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)
as style guideline, we wanted to go for if (!A) rather than if (A ==
NULL). Although I am not sure if the whole codebase was cleaned up yet
or not.
Cheers,
+{
+close(fd);
+return false;
+}
ifcr.ifcr_count = ifcr.ifcr_total;
ifcr.ifcr_buffer = buf;
--
Antonio Quartulli
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] src/openvpn/dco_freebsd.c: handle malloc failure
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
> ---
> 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 abeb..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.)
Fwiw: I usually don’t bother handling malloc failure in userspace, because
modern systems all overallocate anyway, so the first thing you know about lack
of memory is the out-of-memory killer terminating you. It’s a policy choice for
the project, so I don’t object to handling it either.
Best regards,
Kristof
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
