[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: Re: problem with strncpy



Hi Bob,

thanks for your comment.

First, we are not calling this function zillion of times, only about 20 times.
But the problem occured only after doing some hours some other things.
We added the loop 
> for(i = 0; i < TABLE_NAME_LENGTH; i++) {
>   table->name[i] = ?\0?;
> }
for debugging.
(We now replaced that for debugging by: bzero(table->name,TABLE_NAME_LENGTH);)

But you are right with your suggestion:
> Whenever I see it in a code review I almost invariably find it there
> by mistaken understanding of what it does.
Never read about that 'feature' of strncpy.

We will use sprintf in future.
Made a grep on sprintf an strncpy on my code.
Found 319 times sprintf and 17 times strncpy.
Will inspect all of this 17 calls.

Thank you very much for this correction.

Yours,
Markus

Software Consulting Markus Bernhardt GmbH
Spieljochstr. 34  Phone: +49-89-420903-14
81825 München     Fax:   +49-89-420903-20
mailto:Markus.Bernhardt@scmb.de
http://www.scmb.de

Bob Proulx <rwp> schrieb am 21.03.2003 07:41:

> 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
> 
> 
> -- 
> To UNSUBSCRIBE, email to debian-ia64-request@lists.debian.org
> with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
>




Reply to: