Re: [Chicken-hackers] [PATCH] Replace unsafe string functions with their safer counterparts

[Peter Bex]

On Tue, Feb 04, 2014 at 10:05:30AM +0100, Christian Kellermann wrote:
> Hi Peter,
> 
> please find my comments inline.
> 
> * Peter Bex <address@hidden> [140203 22:30]:
> > After some reconsideration, I think this is probably a good idea.
> > The big swath of OpenBSD license text for these rather small functions
> > in our chicken.h really disagreed with me (the license is barely smaller
> > than the code!), so I looked around a bit for public domain
> > implementations and found a few.  I decided to use the one by
> > C.B. Falconer, as it seems to have seen the most review.
> 
> Thanks for considering the patch and taking the time to go through
> it.
> 
> If you are concerned with the big license in the code, I will move
> it to LICENSE. I don't know why exchanging the code solves
> this issue, while an even simpler change would do so as well.

Yes, that's fine. I'm tired of arguing about this, and I know I can't
win.  I prefer getting the patch in over arguing about it, and nobody
else seems to have an opinion about this.

> I even think the PD version of strlcat has an issue: It assumes
> that the size argument for the dst string is actually larger than
> the strlen(dst). I don't see why this should be a valid assumption
> as this does again provide a potential pitfall when using it.
> 
> The OpenBSD version does not trust the size parameter and adjusts
> the length first, then copying the data.

That's a similar issue as to the NULL src argument: not something we
want to accidentally come to rely on, as not all implementations are
certain to do this.  In some sense this actually promotes writing
broken code.  However, making an assertion out of this could be a
good idea.

> Thank you for testing it on windows!
> 
> The refactoring should be a different patch IMHO that should go in before.
> Mixing these things does not make it easier to test, and this is quite a
> change and deserves more than a side note in a commit with a different topic.

That would be an awful lot of work to do for little gain: I tested the
patch on Windows as-is, and it solves several issues with the build.
I don't enjoy this, you know.  Going back in and picking apart the patch
just to reassemble it and test it repeatedly until it works again sounds
painful.

Alternatively, you could do this yourself and test it in a VM on a freely
(as in beer) available copy of Windows provided for "testing purposes" by
Microsoft at http://modern.ie

They don't specify the kind of testing, so it's legal to use for our purposes.

> As I see it apart from the POSIX unit issue by default the brought
> in replacements are used, so I won't expect issues with that.

I'm not sure I understand what you mean by that.

> So my proposal is:
> 
> I will rework my patch and move the license into LICENSE, you will
> move the refactoring of posix into a separate patch and we will
> throw the code into the salmonella pit and ask for testers?

Could you instead just replace the strlcat code in my version and push
the entire thing?

Cheers,
Peter
-- 
http://www.more-magic.net