Re: strncpy is not a safe version of strcpy - Mailing list pgsql-hackers

On 15 Listopad 2013, 1:00, David Rowley wrote:
> On Fri, Nov 15, 2013 at 12:33 PM, Tomas Vondra <tv@fuzzy.cz> wrote:
>
>> > It is likely far better explained here -->
>> > http://www.courtesan.com/todd/papers/strlcpy.html
>> >
>> > For example , the following 2 lines in jsonfuncs.c
>> >
>> > memset(name, 0, NAMEDATALEN);
>> > strncpy(name, fname, NAMEDATALEN);
>>
>> Be careful with 'Name' data type - it's not just a simple string buffer.
>> AFAIK it needs to work with hashing etc. so the zeroing is actually
>> needed
>> here to make sure two values produce the same result. At least that's
>> how
>> I understand the code after a quick check - for example this is from the
>> same jsonfuncs.c you mentioned:
>>
>>     memset(fname, 0, NAMEDATALEN);
>>     strncpy(fname, NameStr(tupdesc->attrs[i]->attname), NAMEDATALEN);
>>     hashentry = hash_search(json_hash, fname, HASH_FIND, NULL);
>>
>> So the zeroing is on purpose, although if strncpy does that then the
>> memset is probably superflous. Either people do that because of habit /
>> copy'n'paste, or maybe there are supported platforms when strncpy does
>> not
>> behave like this for some reason.
>>
>>
> I had not thought of the fact the some platforms don't properly implement
> strncpy(). On quick check http://man.he.net/man3/strncpy seems to indicate
> that this behaviour is part of the C89 standard. So does this mean we can
> always assume that all supported platforms always 0 out the remaining
> buffer?

I don't know about such platform - I was merely speculating about why
people might use such code.

>> I seriously doubt this inefficiency is going to be measurable in real
>> world. If the result was a buffer-overflow bug, that'd be a different
>> story, but maybe we could check the ~120 calls to strncpy in the whole
>> code base and replace it with strlcpy where appropriate.
>>
>>
> The example was more of a demonstration of wrong assumption rather than
> wasted cycles. Though the wasted cycles was on my mind a bit too. I was

Yeah. To be fair, number of occurrences in the code base is not a
particularly exact measure of the impact - some of those uses might be
used in code paths that are quite busy.

> more focused on trying to draw a bit of attention to commit
> 061b88c732952c59741374806e1e41c1ec845d50 which uses strncpy and does not
> properly set the last byte to 0 afterwards. I think this case could just
> be
> replaced with strlcpy which does all this hard work for us.

Hmm, you mean this piece of code?
  strncpy(saved_argv0, argv[0], MAXPGPATH);

IMHO you're right that's probably broken, unless there's some checking
happening before the call.

Tomas