[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: [PATCH] udhcpc: fix segmentation fault on empty bin opt
From:       Michał_Kazior <kazikcz () gmail ! com>
Date:       2019-09-26 9:47:02
Message-ID: CABvG-CWAxpNc+qUN1SQbHGL9oqZYP20HZvEj5G1GdysamLnXKw () mail ! gmail ! com
[Download RAW message or body]

On Wed, 25 Sep 2019 at 14:28, Tito <farmatito@tiscali.it> wrote:
> On 9/25/19 2:03 PM, Michal Kazior wrote:
> > From: Michal Kazior <michal@plume.com>
> >
> > The following caused udhcpc to segfault:
> >    busybox udhcpc -i lo -s /dev/null -x 0x3d:
> >
> > Signed-off-by: Michal Kazior <michal@plume.com>
> > ---
> >   networking/udhcp/common.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/networking/udhcp/common.c b/networking/udhcp/common.c
> > index 4a452cdb9..9ec752dfc 100644
> > --- a/networking/udhcp/common.c
> > +++ b/networking/udhcp/common.c
> > @@ -539,7 +539,7 @@ int FAST_FUNC udhcp_str2optset(const char *const_str, void *arg,
> >
> >               if (optflag->flags == OPTION_BIN) {
> >                       val = strtok(NULL, ""); /* do not split "'q w e'" */
> > -                     trim(val);
> > +                     if (val) trim(val);
> >               } else
> >                       val = strtok(NULL, ", \t");
> >               if (!val)
> >
>
> Hi,
>
> wouldn't it make more sense to put this in libbb/trim.c so it is fixed definitely?

I'm a little uneasy to patch trim(). It seems convenient, but it also
gives an impression that perhaps other functions should be defensively
NULL checked as well, which not many of them seem to be (if any)
today. It'd end up with inconsistent function behavior with regards to
NULL handling. And you'd still need to do NULL checks afterwards
anyway (either the argument, or return value).

While perhaps not the best example, libc doesn't try to be too smart
about NULL checks for string functions and leaves that to the caller.
trim() seems to be intended as libc-like primitive for string
manipulation.

I'm neither in favor nor against at this point. I don't know what the
design principles are for busybox well enough. I can resubmit a v2 if
you wish.


Michal
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[prev in list] [next in list] [prev in thread] [next in thread]