[OpenAFS] Re: sprintf -> snprintf...

[R. Lindsay Todd]

[Coming late to this discussion, since I'm not on -info]

As other have pointed out, there is an snprintf.c implementation in 
src/util.  This defines afs_vsnprintf and afs_snprintf, as well as 
vsnprintf and snprintf for systems that do not already have these.  This 
is a "fairly complete" implementation -- it doesn't support the i18n 
feature of specifying positional parameters, some of the newer ISO 
descriptions, or correct return count if the buffer is too small.  But 
it does support features not found in every sprintf, such as %llu and 
%lld formats for 64-bit integers.

In trying to add LFS to the fileserver, I've needed to be able to 
display integers that might be either 32- or 64-bit, depending on 
compilation options.  Having to choose formats based on compilation 
options becomes messy.  The cleanest solution I've found, and what I've 
implemented, is to make cast these values to afs_intmax_t or 
afs_uintmax_t, which are guaranteed to be correctly formatted using %lld 
or %llu, respectively, whether or not the AFS_64BIT_ENV is set.  But the 
best way to guarantee this is to control the implementation of 
afs_vsnprintf, especially since we will need something like this on some 
architectures anyway.

A number of changes to CVS lately have converted uses of sprintf to 
afs_snprintf in the fileserver and volserver.  It turns out that some of 
the uses of sprintf are problematic: There are cases of %Ld being used 
-- I believe that is a GNU extension.  I found one buffer overrun 
involving deleting volumes...  Who knows what else lurks?  So it seems 
to me that moving to snprintf and similar is necessary to ensure safety.

The question at hand is then, to attempt to use a system snprintf, or 
always use our own afs_snprintf?  I think that most of the time, when 
snprintf is used, it is to generate a string that will be used next to 
log a message, or as a file name for a file system operation.  The 
system snprintf might, maybe, be a little more efficient, but I'm not 
sure it matters in this context.  Meanwhile, we have to maintain our own 
snprintf implementation for systems that don't have  it, and we have to 
maintain a test program to verify that the system snprintf is robust 
enough, and to get correct types of afs_intmax_t.  That seems like a lot 
of overhead for (maybe) marginal gain.

So it seems to me that we should always use our own afs_snprintf.  The 
only question I have: Is the version we have in src/util robust enough 
to replace sprintf throughout the codebase? It seems to be for src/vol, 
src/volser, and src/viced, but I haven't gone through other parts of the 
code.

/Lindsay

Russ Allbery wrote:

>Switching to devel (should have done that before when I started attaching
>code).
>  
>

-- 
R. Lindsay Todd                      email: toddr@rpi.edu
Senior Systems Programmer            phone: 518-276-2605
Rensselaer Polytechnic Institute     fax:   518-276-2809
Troy, NY 12180-3590                  WWW:   http://www.rpi.edu/~toddr