Re: problem with strncpy

[rwp@hprwp.fc.hp.com (Bob Proulx)]

Markus Bernhardt wrote:
> 
> void table_set_param(Table *table, char *name, int count, long index) {
>   int i;
>   for(i = 0; i < TABLE_NAME_LENGTH; i++) {
>     table->name[i] = '\0';
>   }
>   strncpy(table->name, name, TABLE_NAME_LENGTH-1);

> This function is called many times in our software. After
> processing some gigabytes of data the strncpy fails reproducable
> exactly one time. All following calls are working.

I don't have any ideas about your problem beyond what the others
posted as other follow ups to your original post.  But I can't hold
back on some other comments about this code.  So don't let this
distract you from digging into your strange case.  But think about
this as a way to improve your code outside of that problem.

You say you are calling this zillions of times.  How big is
TABLE_NAME_LENGTH?  Big?  Small?  Because in this routine you are
walking the table completely front to back twice.  One in your for
loop and once in your strncpy.  Another post suggested replacing your
for loop with memset and that sems right to me in general.  Definitely
the place for memset() and not a for loop.  But in this case it is
completely unneeded because of the strncpy() right after which is also
walking the database completely.  It renders the first pass
redundant.  (But I would remove the strncpy().)

The second time the table is completely walked is strncpy().  There is
a common misconception about strncpy() that it stops copying at the
end of the string.  But that is not true.  The strncpy() routine
always copies size 'n' bytes.  Which means you are copying zeros again
over the table for a second pass.  The strncpy() routine always copies
the entire size passed to it.  An unfortunate behavior but so it is.

  http://www.UNIX-systems.org/single_UNIX_specification_v2/xsh/strncpy.html

    The strncpy() function copies not more than n bytes (bytes that
    follow a null byte are not copied) from the array pointed to by s2
    to the array pointed to by s1.  If copying takes place between
    objects that overlap, the behaviour is undefined.

    If the array pointed to by s2 is a string that is shorter than n
    bytes, null bytes are appended to the copy in the array pointed to
    by s1, until n bytes in all are written.

Therefore strncpy() is almost never the right routine to use.
Whenever I see it in a code review I almost invariably find it there
by mistaken understanding of what it does.  Most people want something
which can be written, somewhat cryptically, using sprintf() as here.
(This is off the top of my head.  Don't criticize me too much if I
made a mistake.)  This old idiom does what most people wish strncpy()
did.  It copies only what is needed, always zero terminates, will not
overflow the buffer.

  sprintf(s1,"%.*s",n-1,s2);

If you have a working snprintf() then the more direct way of doing
this is using it.  But snprintf() has had a troubled start in life and
different systems have implemented it differently.  Be cautious.  But
it is the better routine to use of the two.

  snprintf(s1,n,"%s",s2);

Back to your code.  If you are doing this a lot then these two passes
are a inefficiency.  Do you have a particular need to zero out the
table past the end of name?  If not then I would do it neither time.
Not once, not twice.  But if you do need it then once should be
enough.

I don't have any understanding of your overall code.  But perhaps this
will be a useful tidbit to think about and might help if not with this
then perhaps with another part.

Bob