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
