Re: [dev] [sic]: patches for string handling

2009/9/23 Kris Maglione <maglione.k_AT_gmail.com>:
> On Wed, Sep 23, 2009 at 02:41:23PM +0100, Anselm R Garbe wrote:
>> 2009/9/23 Kris Maglione <maglione.k_AT_gmail.com>:
>>>
>>> On Wed, Sep 23, 2009 at 12:53:35AM +0200, Mark Edgar wrote:
>>>>
>>>> * Fix buffer overrun when using strncpy()
>>>
>>> It's really sad to see code using strncpy.
>>
>> Well strncpy has performance issues on large buffers since it pads the
>> remainder with null bytes. I agree that it would be better to use
>> something else, though I don't like your util.c strlcpy solution
>> because that's calling strncpy as well and does some superflous
>> double-termination that seems to be unnecessary to me.
>
> Unfortunately, the double-termination isn't superfluous because strncpy
> doesn't always terminate. Just setting the buffer size 1 byte short doesn't
> help unless the last byte is already null. I actually chose to use strncpy
> for LOC reasons, but memcpy isn't much longer:

Well you are right, I was assuming the sic.c context where all buffers
are static and now used in a way that the last byte is always
emergency terminator.

> strlcpy(char *dest, char *src, int len) {
>        int n;
>
>        n = strlen(src);
>        memcpy(dst, src, n < len ? n : len);
>        if(len > 0)
>                dst[len-1] = '\0';
> }
>
> I'd prefer:
>
> strlcpy(char *dest, char *src, int len) {
>        memcpy(dst, src, min(len, strlen(src)));
>        dst[n-1] = '\0';
> }
>
> but then we'd need min.

I prefer the memcpy version, also because that isn't padding remainder
NUL bytes.

>> I applied both sic.c and util.c temporarily to sic/kris/, I need to
>> sort out what I'm going to re-use and what not. The state when you
>> patched sic.c seems to be quite old though... or your patch was quite
>> big ;)
>
> Yes, it's fairly old, I just didn't get the chance to sync it with tip
> before I posted it since some of the changes are a bit dramatic.
>
>> Well apparently they push the line count to 296
>
> Really?
>
> %sloccount *.c
> ...
> SLOC    Directory       SLOC-by-Language (Sorted)
> 252     top_dir         ansic=252
> ...
> Total Physical Source Lines of Code (SLOC)                = 252
>
> I shortened it to 251 because I #included util.c
>
> It could actually be made quite a few lines shorter by, e.g., not
> automatically converting unknown commands to raw commands, etc.

Ah ok, you SLOC'ed it, I just checked with wc -l...

Kind regards,
Anselm
Received on Wed Sep 23 2009 - 14:56:55 UTC