On Fri, Dec 18, 2020 at 01:46:49PM +0100, Theo Buehler wrote:
> On Fri, Dec 18, 2020 at 11:43:40AM +0100, Claudio Jeker wrote:
> > On Wed, Dec 02, 2020 at 05:06:28PM +0100, Claudio Jeker wrote:
> > > rpki-client passes both empty strings and NULL strings as zero length
> > > objects. The unmarshal code then allocates memory in any case and so a
> > > NULL string is unmarshalled as empty string. This is not great, currently
> > > there are no empty strings but a fair amount of NULL strings.
> > > This diff changes the behaviour and now NULL is passed as NULL.
> > > This should simplify passing optional strings (e.g. in the entity queue
> > > code).
> >
> > I will commit this later today. It will help with some further cleanup of
> > the codebase.
>
> I'm a bit torn about this. Some of the callers clearly do not expect
> having to deal with NULL.
>
> I see some *printf("%s", NULL) (for example in proc_rsync()) that should
> never happen but can now happen with this change. I'm unsure that there
> are no NULL derefs that this introduces. I'm fine with this going in if
> you intend to address this as part of the further cleanup work you
> envision.
>
In most cases the code expects a non-empty string. For example in the
rsync.c case neither host nor mod are allowed to be NULL or "".
I guess adding an assert(host && mod) would be enough there.
I actually prefer the code to blow up since as mentioned
empty strings are almost always wrong (e.g. empty filenames or hashes).
Indeed in all those cases a check for NULL should be added in the
unmarshal function.
--
:wq Claudio
