I've got a patch that changes f_name_to() to return an unsigned int
(like snprintf() and strlcpy() do) and adds checking to ensure that we
didn't overflow the name before we try to use it:
http://www.blorf.net/name-overflow.patch
If anyone would care to check out the following patch before I commit
it, please do.
..wayne..
On Mon, Jan 19, 2004 at 12:21:48PM -0800, Wayne Davison wrote:> On Mon, Jan 19, 2004 at 12:05:16PM -0800, jw schultz wrote: > > How about posting it? > > To the mailing list? I think that most of the subscribers aren't going > to be interested in random patches, so it's more space- and bandwidth- > efficient to let people grab a copy if they actually want one.I disagree. This is the developers list not a how-to list. Non-developers are welcome to observe and learn. -- ________________________________________________________________ J.W. Schultz Pegasystems Technologies email address: jw@pegasys.ws Remember Cernan and Schmitt
On Mon, Jan 19, 2004 at 10:17:30AM -0800, Wayne Davison wrote:> I've got a patch that changes f_name_to() to return an unsigned int > (like sme_tonprintf() and strlcpy() do) and adds checking to ensure that we > didn't overflow the name before we try to use it: > > http://www.blorf.net/name-overflow.patch > > If anyone would care to check out the following patch before I commit > it, please do.I don't like it on two counts. If we are going to vet the path name for overflow (a good idea) lets do it once, explicitly, as we receive it instead of having tests scattered throughout the code. In other words, test for strlen(file->dirname) + strlen(file->basename) >= MAXPATHLEN - 2 in receive_file_entry(). When all you are doing is concatinating a couple of strings snprintf is overkill, especially in an infrastructure function. As it is now f_name_to() is about as good as it gets. We went through several iterations with get_tmpname() to keep it clean and efficient there is no need to regress here. -- ________________________________________________________________ J.W. Schultz Pegasystems Technologies email address: jw@pegasys.ws Remember Cernan and Schmitt
On Mon, Jan 19, 2004 at 07:09:29PM -0800, Wayne Davison wrote:> On Mon, Jan 19, 2004 at 06:46:48PM -0800, jw schultz wrote: > > If you're going to do the strlen(src) and whatnot you might > > as well just snag the strlcpy source and tweak it so you > > only have to scan the data once. > > I snagged rsync's version of strlcpy() from the lib/compat.c file. > I just tried the BSD version, and it's 2.33 times slower on my Linux box > (the strlen() and memcpy() functions must be using the string-oriented > processor functions, but the BSD version is not).The memcpy may outperform a loop if it uses the gcc builtin. As long as the string is short the strlen will just preload the L1, if it is long it will thrash the L1. One possiblility would be to not do the strlen on the first src arg which tends to be a dirname and will be pre-vetted for length.> > If you are going to plug in this low level function, why not do a > > version that does path joining so users don't have to provide the "/". > > Just because I thought this was more generic, and there are several > sections of code that join strings without including a "/". However, > the joining of two strings with a '/' in between is certainly the most > common join, so it might be worth a dedicated function just for that > idiom.I wasn't saying "instead" but "also".> > > Do we really have such a variety of callers that need the overhead of > > varargs? > > In my timings it wasn't much of a hit -- in 900,000 iterations it caused > an extra ~.03 user-seconds of a total of ~1.30 user-seconds. > > FYI, the rsync code currently has string-joins that range from 2 - 4 items. > > ..wayne.. >-- ________________________________________________________________ J.W. Schultz Pegasystems Technologies email address: jw@pegasys.ws Remember Cernan and Schmitt