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

[Christian Kellermann]

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.

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.

> Finally, to make it work on MingW I had to fix quite a few more things
> than I expected, so I present my altered copy containing these fixes for
> re-review:
> 
> - The C_setenv fallback implementation for when C_GNU_ENV is unset
>    (a bit of a bogus check, if you ask me) contained swapped length
>    and buffer arguments, causing a segfault upon the first (setenv )
>    call I made.  I fixed the swapped calls and moved it to
>    posix-common.scm because Windows uses the exact same fallback
>    implementation (and C_GNU_ENV is unset in MingW).  The Scheme side
>    of these functions is also completely identical, so I moved it
>    as well - this seems like a huge change, but it should prevent future
>    divergence.

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.

> - The Windows implementation of the POSIX unit broke due to the change
>    in foundfile()'s "prototype" which is now expected by the Scheme
>    code in posix-common, but wasn't carried on to posixwin.scm.
>    I moved your version to posix-common.scm as the original macros
>    were identical across both platform-specific files.  Same for
>    C_opendir, C_closedir and C_readdir.
> - In fact, posixwin.scm still contained several calls to C_strcat() and
>    C_strcpy() (which you removed from chicken.h), so after fixing the
>    compilition, it failed to to link.  I've updated it to use the safe
>    versions too.

Again thanks!

I did not test this on windows due to lack of access. I
deliberately removed those references so accidental usage will fail
hard and reliably for exactly this scenario. I also found the string
functions "hidden" in the TCP unit this way.

> - You signed off on your own patch :)  I removed this line from the
>    commit message so you can sign off again, on my modifications.  That
>    way, we'll know that the change which carries both our signoff lines
>    is the correct one.

Yes I signed it off as I wrote it and tested it. Sorry, this habit
leaks in from other communities. I will try to refrain from doing
so in CHICKEN in the future.

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.

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?

Kind regards,

Christian

-- 
In the world, there is nothing more submissive and weak than
water. Yet for attacking that which is hard and strong, nothing can
surpass it. --- Lao Tzu