Re: [Openvpn-devel] [PATCH] cppcheck finding: add "ASSERT( maxoutput > 0 || separator != NULL )" to prevent possible null pointer derefence
> > Hello, > > this defect was found by cppcheck, however cppcheck still complains. > so, we did not make it happy yet. > > I think, the best would be split this function into 2 separate functions > (with either null argument) or leave it like that. Unrelated to an actual fix, IMHO we should at least break up out = alloc_buf_gc (maxoutput ? maxoutput : ((size * 2) + (size / (space_break_flags & FHE_SPACE_BREAK_MASK)) * (int) strlen (separator) + 2), gc); and create a variable with a semantic variable for the result of "((size * 2) + (size / (space_break_flags & FHE_SPACE_BREAK_MASK)) * (int) strlen (separator) + 2)” Honestly the calculation looks very arcane and I would better understand it, if i knew what it means :-) Jens-- ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] cppcheck finding: add "ASSERT( maxoutput > 0 || separator != NULL )" to prevent possible null pointer derefence
Hello,
this defect was found by cppcheck, however cppcheck still complains.
so, we did not make it happy yet.
I think, the best would be split this function into 2 separate functions
(with either null argument) or leave it like that.
2016-09-18 20:14 GMT+05:00 David Sommerseth <
open...@sf.lists.topphemmelig.net>:
> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA1
>
> On 18/09/16 17:19, Ilya Shipitsin wrote:
> > --- src/openvpn/buffer.c | 6 -- 1 file changed, 4
> > insertions(+), 2 deletions(-)
> >
> > diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c index
> > 52c6ab9..57bded9 100644 --- a/src/openvpn/buffer.c +++
> > b/src/openvpn/buffer.c @@ -438,10 +438,12 @@ format_hex_ex (const
> > uint8_t *data, int size, int maxoutput, unsigned int
> > space_break_flags, const char* separator, struct gc_arena *gc) { -
> > struct buffer out = alloc_buf_gc (maxoutput ? maxoutput : + int i;
> > + struct buffer out; + ASSERT( maxoutput > 0 || separator != NULL
> > ); + out = alloc_buf_gc (maxoutput ? maxoutput : ((size * 2) +
> > (size / (space_break_flags & FHE_SPACE_BREAK_MASK)) * (int) strlen
> > (separator) + 2), gc); - int i; for (i = 0; i < size; ++i) { if
> > (separator && i && !(i % (space_break_flags &
> > FHE_SPACE_BREAK_MASK)))
> >
>
> This looks dangerous and somewhat wrong. This can actually stop a
> server completely if the ASSERT() check fails. I'm not sure we want
> to do that.
>
> The ASSERT() also expects maxoutput to be higher than 0, while the
> alloc_buf_gc() supports/allows maxoutput to be 0, which again confirms
> my bad feeling about this. If maxoutput == 0, alloc_buf_gc() will
> actually allocate a reasonable buffer regardless.
>
> I do see that there might be an issue if separator == NULL
> (strlen(NULL) will segfault), so this needs to be checked far more
> carefully and compared against all the callers of format_hex_ex().
>
> The question which pops up in my head is: Is this patch purely
> targeted to silence a code analyser warning? If so, this is most
> likely not the right fix for the OpenVPN code base.
>
>
> - --
> kind regards,
>
> David Sommerseth
> OpenVPN Technologies, Inc
>
> -BEGIN PGP SIGNATURE-
> Version: GnuPG v2.0.22 (GNU/Linux)
>
> iQIcBAEBAgAGBQJX3q9BAAoJEIbPlEyWcf3yGkwQAKKZbI2idz6a7h5ZkbQJCHMk
> JDNIpdR6XTFmG16jBrycYeew+9IRvfwpF4vh2/QPaRjDNLnKvPHP+9uEetqYmUQi
> rWzJyg3nzXSEUhFRSbdJcOK7zEWoiUUsSTiBKIYP6D5qD+QBQOZaQseCCzafrt5b
> IKrUesWMWPXAjQBYbUvcc801+JR5S/gSaSbk9TuO8Nemiry/GW/pSt0OVTggPdcm
> LiUyutQ8HXeXS9uAL32FLqFH3qq7QTHjxdVoKGGx9w/lmznqp+EaKCuANsWnt/PW
> ZTzVYM5YG5jI8XNM/wyCklu5n3yqJ37Noxd3ZRSD6ATTL/MExVgWINOOGA7f+Pj3
> P6g4ZSc//zaWUCRpUKg4O8h6jaO+0stDS5eKDpyUCHYRHRdMI4IGLuwzIzKmxWq9
> LpNGGY3Ikpf5IM+i2R04HctnNp+izEeW3vBCKnBBUHSyegRQGQ191Z5QC3SPk6VP
> HwxkHCZiOsie8WsP0Sl2dur1GjV9uyslVTkmt1GBVuoOGxpmZLJRJRayO70goZ17
> ToavpadDjt193g79j1/dpE7OCPcCitso/+egxVzDWPTtLj+7rS/gfdJBNdCHr7xK
> Pf8KKbpD7yEbEln0K7wzsjVG749qZe103KmrhZulvu3H5+SrbT5ALhroaYidhRpg
> lLcx4g4A+nZjM6kabBGy
> =QvxV
> -END PGP SIGNATURE-
>
--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] cppcheck finding: add "ASSERT( maxoutput > 0 || separator != NULL )" to prevent possible null pointer derefence
Hi, On Sun, Sep 18, 2016 at 06:14:09PM +0300, David Sommerseth wrote: > This looks dangerous and somewhat wrong. This can actually stop a > server completely if the ASSERT() check fails. I'm not sure we want > to do that. This was discussed in a github PR, but I do not have the number right now. Basically, we *know* that we always call this like this, but if we should ever call it differently, the alloc_buf_gc() call will lead to funnies - thus, better ASSERT() in that case, so it's clear the calling contract has been violated. > I do see that there might be an issue if separator == NULL > (strlen(NULL) will segfault), so this needs to be checked far more > carefully and compared against all the callers of format_hex_ex(). See PR :) (And yes, if things are discussed in a PR and patches get then sent to the list months(!) later, it would be useful to actually mention the PR in the commit message - "commit --amend" before sending) gert -- USENET is *not* the non-clickable part of WWW! //www.muc.de/~gert/ Gert Doering - Munich, Germany g...@greenie.muc.de fax: +49-89-35655025g...@net.informatik.tu-muenchen.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] cppcheck finding: add "ASSERT( maxoutput > 0 || separator != NULL )" to prevent possible null pointer derefence
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On 18/09/16 17:19, Ilya Shipitsin wrote:
> --- src/openvpn/buffer.c | 6 -- 1 file changed, 4
> insertions(+), 2 deletions(-)
>
> diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c index
> 52c6ab9..57bded9 100644 --- a/src/openvpn/buffer.c +++
> b/src/openvpn/buffer.c @@ -438,10 +438,12 @@ format_hex_ex (const
> uint8_t *data, int size, int maxoutput, unsigned int
> space_break_flags, const char* separator, struct gc_arena *gc) { -
> struct buffer out = alloc_buf_gc (maxoutput ? maxoutput : + int i;
> + struct buffer out; + ASSERT( maxoutput > 0 || separator != NULL
> ); + out = alloc_buf_gc (maxoutput ? maxoutput : ((size * 2) +
> (size / (space_break_flags & FHE_SPACE_BREAK_MASK)) * (int) strlen
> (separator) + 2), gc); - int i; for (i = 0; i < size; ++i) { if
> (separator && i && !(i % (space_break_flags &
> FHE_SPACE_BREAK_MASK)))
>
This looks dangerous and somewhat wrong. This can actually stop a
server completely if the ASSERT() check fails. I'm not sure we want
to do that.
The ASSERT() also expects maxoutput to be higher than 0, while the
alloc_buf_gc() supports/allows maxoutput to be 0, which again confirms
my bad feeling about this. If maxoutput == 0, alloc_buf_gc() will
actually allocate a reasonable buffer regardless.
I do see that there might be an issue if separator == NULL
(strlen(NULL) will segfault), so this needs to be checked far more
carefully and compared against all the callers of format_hex_ex().
The question which pops up in my head is: Is this patch purely
targeted to silence a code analyser warning? If so, this is most
likely not the right fix for the OpenVPN code base.
- --
kind regards,
David Sommerseth
OpenVPN Technologies, Inc
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.22 (GNU/Linux)
iQIcBAEBAgAGBQJX3q9BAAoJEIbPlEyWcf3yGkwQAKKZbI2idz6a7h5ZkbQJCHMk
JDNIpdR6XTFmG16jBrycYeew+9IRvfwpF4vh2/QPaRjDNLnKvPHP+9uEetqYmUQi
rWzJyg3nzXSEUhFRSbdJcOK7zEWoiUUsSTiBKIYP6D5qD+QBQOZaQseCCzafrt5b
IKrUesWMWPXAjQBYbUvcc801+JR5S/gSaSbk9TuO8Nemiry/GW/pSt0OVTggPdcm
LiUyutQ8HXeXS9uAL32FLqFH3qq7QTHjxdVoKGGx9w/lmznqp+EaKCuANsWnt/PW
ZTzVYM5YG5jI8XNM/wyCklu5n3yqJ37Noxd3ZRSD6ATTL/MExVgWINOOGA7f+Pj3
P6g4ZSc//zaWUCRpUKg4O8h6jaO+0stDS5eKDpyUCHYRHRdMI4IGLuwzIzKmxWq9
LpNGGY3Ikpf5IM+i2R04HctnNp+izEeW3vBCKnBBUHSyegRQGQ191Z5QC3SPk6VP
HwxkHCZiOsie8WsP0Sl2dur1GjV9uyslVTkmt1GBVuoOGxpmZLJRJRayO70goZ17
ToavpadDjt193g79j1/dpE7OCPcCitso/+egxVzDWPTtLj+7rS/gfdJBNdCHr7xK
Pf8KKbpD7yEbEln0K7wzsjVG749qZe103KmrhZulvu3H5+SrbT5ALhroaYidhRpg
lLcx4g4A+nZjM6kabBGy
=QvxV
-END PGP SIGNATURE-
--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
