The dangers of static buffers in rsync code

jw schultz jw at pegasys.ws
Tue Dec 30 10:01:35 EST 2003


On Mon, Dec 29, 2003 at 04:55:19PM -0500, Cristian Gafton wrote:
> 
> I have been trying for quite a while now to understand why is the 
> flist.c:f_name() function implemented using static buffers. Anyone care to 
> comment?
> 
> The immediate problem is that any call to f_name overrides the previous
> content (well, obvious). This, combined with the fact that several
> function calls are made with the result of f_name(file) results in
> problems handling hardlinks - and possibly other issues as well, but
> hardlinks is what made me look deeper inside the rsync code.
> 
> As a side note, the f_name function has some "protection" against this
> static clobbering: instead of having a single buffer, it has... TEN, and
> it cycles through them. That's both funny and sad...
> 
> One obvious problem path is starting from generator.c:generate_files()  
> function. In there we call in a while loop:
> 
> recv_generator(local_name?local_name:f_name(file),flist,i,f), which calls
>   hlink.c:check_hard_link(), which calls
>     hlink.c:hlink_compare(), which calls
>       flist.c:file_compare(), which calls
>         flist.c:f_name().
> 
> Interested parties might want to follow the code, it is easier and more
> obvious...
> 
> Anyway, the end result is that in recv_generator() the first argument 
> (fname) is now clobbered by our search through the hlink_list, and 
> depending on what was the last thing we put in there there are various 
> failures happening.
> 
> The bad part is that they are mostly silent. Most of the bitching is done
> via "if (verbose > 1)", so with the standard 'rsync -av' there are no
> indicators that would tell somebody rsync failed to handle a particular
> file...
> 
> One way of "unclobbering" this is by forcing another call to f_name after
> the check_hard_link() call is finished (wither in check_hard_link itself
> or in recv_generator), but that smells rather nasty... On the other hand,
> reimplementing f_name to make it sane would require some bigger changes,
> since f_name() happens to be quite a popular function inside the rsync
> code...
> 
> This is bad and needs some fixing... Little hammer or big hammer?

How about an even smaller hammer.
In recv_generator do a strlcpy of arg1 to an automatic.

-- 
________________________________________________________________
	J.W. Schultz            Pegasystems Technologies
	email address:		jw at pegasys.ws

		Remember Cernan and Schmitt



More information about the rsync mailing list