[Bug-tar] Fix for the warnings in 1.20

[Andrey V. Stolyarov]

Dear colleagues,


I've noticed that tar-1.20 produces a couple of warnings during
build, and decided to try fixing them.  Here are my results and
comments.

I attach the patch (taken with diff -Nurp), which contains the
fixes that I suggest.


# xheader.c: In function `xheader_format_name':
# xheader.c:240: warning: 'pptr' might be used uninitialized in this function

This function, being given a string containing several possible %-escapes,
builds a new string, in which the escapes are replaced by certain values.

The function consists of two cycles through the pattern string, the first
cycle computes the length of the resulting string, and the second actually
builds the string.

The variable 'pptr' is only used by '%p' escape, which, according to the
comment, is to be replaced by the process id of the pax process.

The problem is, in fact, that the 'pptr' is initialized within the first
cycle, while is used in the second.  Definitely it can not get used unless
the '%p' escape is found in the string, in which case 'pptr' is already
initialized in the first cycle; however, it is not that obvious for the
compiler (okay, such a problem is Turing-unsolvable).

This could be fixed making the variable 'pptr' "more local", that is,
initializing it immediately before its use, but this is not good for
(rare?) cases when the '%p' escape is found more than one time.  So,
I believe the best way of fixing this is just to add initialization
to the 'pptr's declaration, which I did.


The second warning was not that obvious.

# incremen.c: In function `dumpdir_create0':
# incremen.c:102: warning: assignment discards qualifiers from pointer target 
type

The line 102 assigns a constant string location to an element of struct
dumpdir::elv array, which is declared simply as 'char **', without any consts.  
I've tried to take an assumption that strings pointed by elements of that 
array are really never changed, and added the 'const' modifier to the field's 
declaration (line 57).  This leads to changing the return types of the 
dumpdir_next function, as it returns a pointer into the location immediately
_before_ that pointed by elv[..].

Leaving aside the fact dumpdir_next is public, within the same module 
this also requires changing the return type of dumpdir_first, as it 
actually returns the value returned by dumpdir_next (line 180).  
And, more importantly, this breaks the (module-local, that is, static)
function scan_directory, which cycles through strings returned by
dumpdir_{first,next} and _modifies_ them.

At this point I decided to stop diving further into this and try to
figure out what's the strange memory area these elv[..]s point to.
So, I reverted all the changes and instead removed the const modifier
from the pointer whose content is assigned to these elv[..]es thus
producing the warning (line 83, within the function dumpdir_create0 --
btw, this function is also public, that is, mentioned in common.h and
isn't static, but actually it is never mentioned in other modules).
This forced me to change the type of the 'contents' parameter of the
function, then to change the same parameter of the function
dumpdir_create, and, finally, of the (static!) function note_directory.
Surprisingly enough, the module has just compiled after that, issuing no
warnings.  This means I was right assuming these parameters always point
to some internal data which is Okay to modify, and the 'const' modifier
in their headers was a kind of nonsense, as the location pointed by
the 'contents' parameter is modified anyway.

I've also made these dumpdir_create functions static and removed their
prototypes from the "common.h" file, as they appear to be only called
by a (static) function within the same module, and nowhere else.


See the corresponding patch in the attached file,
and thank you for the great job you do!

--
Andrey V. Stolyarov a.k.a. Croco
Openwall, Inc.

tar-1.20-warnings.diff
Description: Text document