On Tuesday 08 December 2009 20:23:35 Josh Fisher wrote:
> Kern Sibbald wrote:
> > On Tuesday 08 December 2009 16:07:42 Josh Fisher wrote:
> >> Is it that strncat()
> >> can write n+1 chars instead of n (like the other strn* functions)?
> > No, the Unix definition of strncat(char *dest, const char *src, size_t
> > n); is that a maximum of n characters may be transferred from src to
> > dest. Thus n serves no use in ensuring that the size of dest is not
> > exceeded. This is not very logical to me and is not the way I would have
> > implemented the function.
> > I believed that n-1 was the maximum number of characters total that dest
> > could hold. This is what bstrncat() was supposed to do. The bacula
> > function is now "correctly" implemented according to what I think is much
> > more useful and logical.
> More than logical, it's absolutely necessary. There simply is no way to
> limit dest size with strncat() as there is with the other strn*
> functions. I asked about the n+1 chars because the man page in Fedora 11
> (dated 2008-06-13) also states:
> "If src contains n or more characters, strncat() writes n+1
> characters to dest (n
> from src plus the terminating null byte). Therefore, the size of
> dest must be at
> least strlen(dest)+n+1."
> So, even worse, strncat() can add n+1 chars onto dest. I thought perhaps
> you were manually limiting n by setting n = sizeof(dest) - strlen(dest)
> or something. Because of forcing the terminating null, it has to be n =
> sizeof(dest) - strlen(dest) + 1. Of course, that is very hackish and
> implementing your own proper string concatenation function is much
> cleaner and easier to understand.
Yes, in addition to the above differences between n and n+1 characters, on
various systems some will not store the terminating EOS, which are the
reasons I spent a very long time in the beginning writing my own basic
routines. It is even worse for snprintf() functions across various flavors of
Unix. Needless to say, Bacula has to deal with that :-(
Unfortunately, I got the strncat() stuff wrong. Fortunately, 99% of the data
handling routines do not use bstrncat(), and where bstrncat() is used, most
don't risk overflow.
> > The old manpage for strcat correctly states the implementation, but I did
> > not read it attentively enough some years ago (this manpage is still in
> > SuSE 10.2). A more recent manpage (with a lot of additions) available
> > on my Ubuntu system makes it pretty clear how the function behaves.
> > I wonder how many other programmers misunderstand the behavior of
> > strncat().
> Judging from the plethora of buffer overrun vulnerabilities that still
> plague us, quite a few.
Yes, Bacula is extremely well protected against malloced buffer overflows. In
this particular case the bstrncat() was trashing the stack *backwards*. I
guess some OS implementations are odd. It took quite a while to track it
down to the cause ... Finally, after eliminating just about every
possibility, I decided to *carefully* read the manpage (on the old SuSE).
What a surprise!
> Most logical beings would expect the destination
> size to be limited, not the source size.
Yes, I fell right into that one. :-( I'll read manpages more carefully in
> After all, what good does it do
> to limit the source size? So strncat() is, by definition, no safer than
> plain old strcat(). It should be deprecated along with a warning that it
> is unsafe in the man page.
Yes, I agree completely. By the way, I checked the Bacula usage of bstrncat()
and all calls expect the size to refer to the maximum size of the destination
string, so now we are OK. There are no longer any strncat() calls in Bacula.