| Message ID | 20230517200143.216-1-chipitsine@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] src/openvpn/dco_freebsd.c: handle malloc failure | expand |
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 <chipitsine@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.) 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
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 <chipitsine@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) 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;
ср, 17 мая 2023 г. в 22:43, Kristof Provost <kp@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 <chipitsine@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;" > > 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 >
ср, 17 мая 2023 г. в 22:47, Antonio Quartulli <a@unstable.cc>: > 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 <chipitsine@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) > > 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 >
On 17 May 2023, at 16:58, Илья Шипицин wrote: > ср, 17 мая 2023 г. в 22:43, Kristof Provost <kp@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 <chipitsine@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. Best regards, Kristof
ср, 17 мая 2023 г. в 23:04, Kristof Provost <kp@freebsd.org>: > On 17 May 2023, at 16:58, Илья Шипицин wrote: > > ср, 17 мая 2023 г. в 22:43, Kristof Provost <kp@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 <chipitsine@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 > > Best regards, > Kristof >
On 17 May 2023, at 17:06, Илья Шипицин wrote: > ср, 17 мая 2023 г. в 23:04, Kristof Provost <kp@freebsd.org>: > >> On 17 May 2023, at 16:58, Илья Шипицин wrote: >>> ср, 17 мая 2023 г. в 22:43, Kristof Provost <kp@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 <chipitsine@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
Hi, On 17/05/2023 23:10, Kristof Provost wrote: > On 17 May 2023, at 17:06, Илья Шипицин wrote: >> ср, 17 мая 2023 г. в 23:04, Kristof Provost <kp@freebsd.org>: >> >>> On 17 May 2023, at 16:58, Илья Шипицин wrote: >>>> ср, 17 мая 2023 г. в 22:43, Kristof Provost <kp@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 <chipitsine@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. > 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
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
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 <chipitsine@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) > > 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).
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
чт, 18 мая 2023 г. в 08:47, Gert Doering <gert@greenie.muc.de>: > 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 > gert@greenie.muc.de >
чт, 18 мая 2023 г. в 09:10, Илья Шипицин <chipitsine@gmail.com>: > > > чт, 18 мая 2023 г. в 08:47, Gert Doering <gert@greenie.muc.de>: > >> 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 >> gert@greenie.muc.de >> >
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.
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) + { + close(fd); + return false; + } ifcr.ifcr_count = ifcr.ifcr_total; ifcr.ifcr_buffer = buf;
malloc was not checked against NULL, I was able to get core dump in case of failure Signed-off-by: Ilya Shipitsin <chipitsine@gmail.com> --- src/openvpn/dco_freebsd.c | 5 +++++ 1 file changed, 5 insertions(+)