Re: Release of version 4.14.2 of sharutils

[Eric Blake]

On 01/02/2015 05:44 PM, Bruce Korb wrote:

> OK, I've incorporated use of base_name and fixed the "const" attribute.

No, you still missed a spot where you need to use base_name; and you
still have some other portability bugs.  Read on...

> Anything else before an official re-release?

I built on 32-bit cygwin, and everything went fine (no build warnings
with the default warning level, all tests passed).

I then repeated on 64-bit cygwin, and did not see any compiler warnings
by default, but still failed one of the tests.  Trying again with '-Wall
-Wextra' added to $CFLAGS, I saw a number of ctype usage bugs that you
ought to fix, although most users these days don't pick locales that
would trigger the buggy behavior:

  CC       libopts_a-libopts.o
In file included from libopts.c:32:0:
makeshell.c: In function 'emit_usage:
makeshell.c:399:13: warning: array subscript has type 'char'
[-Wchar-subscripts]
             if ((*pzPN++ = (char)tolower(*pz++)) == NUL)
             ^
and several similar warnings (line 656, 659, 908 in makeshell.c, also
line 1583, 1584, 1595, 1569, 2082, 2091 of shar.c)

Here's an explanation of why this is indeed a (corner-case) bug - 'char'
on cygwin is a signed type, which means tolower((char)255) is
indistinguishable from tolower(-1) (due to the way sign-extension of
signed char is defined), but cygwin's definition of EOF is -1, so it
must return EOF.  However, it is feasible to have a single-byte locale
where character 255 has different properties than EOF (that is, where
tolower((unsigned char)255) returns some other positive value).
Similarly, calling tolower((char)254) behaves as if it were tolower(-2)
which is undefined behavior according to POSIX (on cygwin, it happens to
have sane semantics of mimicking tolower((int)254), because cygwin
happens to map the lookups relative to the middle of a 384-byte table
where offsets [-128..-2] mirror [128-254] such that negative offsets
give the same results as their positive counterpart - only EOF has to
behave differently than 255) but that is not universally guaranteed on
other platforms).

Thus, when using tolower() and other <ctype.h> functions (which are all
locale-sensitive), you absolutely must convert the input argument
(*pz++) to unsigned char before calling the function.  I suggest looking
at coreutils' src/system.h:to_uchar() as a nicer way to coerce ctype
arguments to the correct type than using plain casts.

Also, remember that tolower() doesn't always play nice in multibyte
locales; you may want to decide if it is right to make this code
locale-independent and multi-byte-safe by using gnulib's <c-ctype.h> and
c_tolower(), although it limits the set of case conversions that will be
performed to just ascii characters even in locales that have other case
mappings defined.

Meanwhile, here's the bug that caused the testsuite failure:

shar.c: In function 'split_shar_ed_file':
shar.c:1535:12: warning: implicit declaration of function 'basename'
[-Wimplicit-function-declaration]
            basename (output_filename), part_number,
            ^
shar.c:1538:12: warning: format '%s' expects argument of type 'char *',
but argument 3 has type 'int' [-Wformat=]
            restore, sharpid);
            ^

Here's another bug that should be fixed (a 64-bit ssize_t can cause the
fprintf to misbehave):

scribble.c: In function 'xscribble_get':
scribble.c:166:9: warning: format '%u' expects argument of type
'unsigned int', but argument 3 has type 'ssize_t' [-Wformat=]
         fprintf(stderr, _("could not allocate %u bytes of scribble
space"), sz);
         ^

Finally, there were several harmless warnings, such as:

shar.c: In function 'format_report':
shar.c:345:25: warning: unused parameter 'type' [-Wunused-parameter]
 format_report(quot_id_t type, char const * fmt, char const * what)
                         ^

where you could use gcc attributes to shut it up (gnulib uses _GL_UNUSED
as a macro that expands to the right thing, if you want to try that), and:

unshar.c:62:41: warning: "/*" within comment [-Wcomment]
  * Check for start of comment ("//" or "/*") and directives.
 ^

except that I don't know how you could reword that comment to still
convey the same information without causing grief to (non-compliant)
compilers that try to parse it as nested comments.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature