[Grub-dev] GrubCClient review [Was: Re: Yet another invalid workunit entry]

[Rainer Blome]

Hey, I didn't know you wrote this! =)

Balinny wrote:
> Rainer Blome wrote:
>> Has Grub been run with a memory debugger?
> Running it under valgrind, there're many errors reported... on ld-2.3.6, 
> by subcalls of gethostbyname :S
Heh.

> No problems on the Client detected.

Before or after the fixes?

>> Has its code been reviewed?
> As far as i know, you may be the first one which bothered to read it.

A youngling this program is then?

>> o String function calls without explicit limits.
> Can you elaborate? AFAIK the only unlimited piece was the strtok (i 
> think i was a bit lazy when i added it), which i just replaced.

Security and laziness do not mix well :-).
Always using strn functions, even when you can prove that they are not 
necessary, makes it easier for readers of the code to see whether it is 
safe.  See below for a non-strn-scan command and its output.


>> o The function FetchPage...
> Yeah, it's a kind of beast. But things are quite enchained. Perhaps the 
> first 25 lines
> could be moved to a validating subfunction, splitting the rest would 
> take a little more.

Separate tasks:
o validate order
o assemble a request
o send the request
o receive a reply
o manage memory during receiving
o evaluate reply


> You still have to evaluate the user support ;)

I am not a user :-P ...........................................yet :-).

o This line has no effect:
      memset(&(their_addr.sin_zero), 8, 0);
   because the second arg is the value and the last arg is
   the number of times.

o "return -1" statements leak one socket each (the socket
   is not closed).

Trying to push it through different compilers...

o Some nitpicking tools do not like //-comments
   after #include directives.
   This makes it harder to run code through nitpicking tools.

o The same goes for variables in the middle of the function.
   Moving these to the beginning of the enclosing block or wrapping
   them in brances helps.

Run GCC with more warning options, in particular -W.
See below for a suggested set of warning options and gcc's output.

Rainer


# ----------------------------------------------------------------------
# Scan the code for non-strn calls
cd c/;
for f in *.c *.h; do
   sed -e's|string|s t r i n g|g;s|strn|s t r n|g' \
     -e 's|struct|s t r u c t|g' < $f > $f-;
   grep -n str $f- /dev/null | sed -e's|.c-:|.c:|';
done;

GrubCClient.c:56:char* chrntok(char* str, size_t n, char chr);
GrubCClient.c:88:    char* HostPortSplit = strchr(Server, ':');
GrubCClient.c:152:        size_t QueryLen = strlen(Server) + 
strlen(Path) + strlen(UserAgent) + sizeof(Petition) - 3*2;
GrubCClient.c:288:            if (!(ContentType = MIME_Verify((signed 
char*)strtok(ContentType, " ;"))))
GrubCClient.c:298:            size_t sLen = strlen(LineBuffer) + 1;
GrubCClient.c:316:    unsigned int len = Error ? strlen(Error) : 0; //We 
should never reach here without Error being set
GrubCClient.c:329://Returns the first token on the first n characters of 
str split by chr
GrubCClient.c:331:char* chrntok(char* str, size_t n, char chr) {
GrubCClient.c:333:    for (i=0; (i < n) && str[i]; i++) {
GrubCClient.c:334:        if (str[i] == chr) {
GrubCClient.c:335:            str[i] = '\0';
GrubCClient.c:336:            return str;
GrubCClient.c:447:                    PutHostname = strdup(HostName);
GrubCClient.c:448:                    PutURL = strdup(URL);
GrubCClient.c:476:        Proxy += 7, strtok(Proxy, "/");
GrubCClient.c:489:    size_t l = strlen(argv[1]);
GrubCClient.c:495:    strcpy(C, CompressCmd);
GrubCClient.c:496:    strcpy(C + sizeof(CompressCmd) - 1, argv[1]);
GrubCClient.c:498:    strcpy(C + sizeof(CompressCmd) - 1 + l, ArcExt);
GrubCClient.c:500:    strcpy(C + sizeof(CompressCmd) - 1 + l, ArcGzipExt);
GrubCClient.c:516:    strcpy(C + sizeof(CompressCmd) - 1 + l + 
sizeof(ArcExt) - 1, GzipExt);
GrubCClient.c:518:    char * Y = strdup(C + sizeof(CompressCmd) - 1);
GrubCClient.c:589:    hlen = strlen(headername);
putarchive.c:51:     char* HostPortSplit = strchr(Host, ':');
putarchive.c:102:     strcpy(Buffer+9, "XXX");


# ----------------------------------------------------------------------
# This was on a 64 bit OS, where size_t is 64 bits and unsigned int is 32.

>         gcc -Wall -W -Wwrite-strings -Wpointer-arith -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wnested-externs -Wconversion -Wundef -Winline -fno-dollars-in-identifiers  -c GrubCClient.c -o GrubCClie\
> nt.o
> GrubCClient.c:9:6: warning: "__STDC_VERSION__" is not defined
> GrubCClient.c:53: warning: function declaration isn't a prototype
> GrubCClient.c:71: warning: function declaration isn't a prototype
> GrubCClient.c: In function 'FetchPage':
> GrubCClient.c:156: warning: format '%u' expects type 'unsigned int', but argument 3 has type 'size_t'
> GrubCClient.c:156: warning: format '%u' expects type 'unsigned int', but argument 4 has type 'size_t'
> GrubCClient.c:164: warning: comparison between signed and unsigned
> GrubCClient.c:165: warning: format '%d' expects type 'int', but argument 3 has type 'size_t'
> GrubCClient.c:165: warning: format '%d' expects type 'int', but argument 4 has type 'size_t'
> GrubCClient.c:239: warning: comparison between signed and unsigned
> GrubCClient.c:248: warning: comparison between signed and unsigned
> GrubCClient.c:264: warning: passing argument 3 of 'recv' as unsigned due to prototype
> GrubCClient.c:286: warning: assignment discards qualifiers from pointer target type
> GrubCClient.c:289: warning: assignment discards qualifiers from pointer target type
> GrubCClient.c:296: warning: format '%u' expects type 'unsigned int', but argument 8 has type 'size_t'
> GrubCClient.c:300: warning: format '%u' expects type 'unsigned int', but argument 7 has type 'size_t'
> GrubCClient.c:304: warning: passing argument 2 of 'ARCWrite' as signed due to prototype
> GrubCClient.c:309: warning: passing argument 3 of 'chrntok' with different width due to prototype
> GrubCClient.c:322: warning: passing argument 2 of 'ARCWrite' as signed due to prototype
> GrubCClient.c: In function 'chrntok':
> GrubCClient.c:333: warning: comparison between signed and unsigned
> GrubCClient.c: In function 'copytoEOL':
> GrubCClient.c:353: warning: comparison between signed and unsigned
> GrubCClient.c:526:6: warning: "DELAY_UPLOAD" is not defined
> GrubCClient.c:537:18: warning: "KEEP_FILES" is not defined
> GrubCClient.c: In function 'getheader':
> GrubCClient.c:590: warning: comparison between signed and unsigned
> GrubCClient.c:594: warning: passing argument 3 of 'strncasecmp' as unsigned due to prototype
> GrubCClient.c:595: warning: comparison between signed and unsigned
> GrubCClient.c:595: warning: comparison between signed and unsigned
> GrubCClient.c:610: warning: comparison between signed and unsigned
> GrubCClient.c:610: warning: signed and unsigned type in conditional expression
>         gcc -Wall -W -Wwrite-strings -Wpointer-arith -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wnested-externs -Wconversion -Wundef -Winline -fno-dollars-in-identifiers  -c ARCFile.c -o ARCFile.o
> ARCFile.c: In function 'ARCWrite':
> ARCFile.c:24: warning: passing argument 3 of 'write' as unsigned due to prototype
> ARCFile.c: At top level:
> ARCFile.c:60: warning: function declaration isn't a prototype
>         gcc -Wall -W -Wwrite-strings -Wpointer-arith -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wnested-externs -Wconversion -Wundef -Winline -fno-dollars-in-identifiers  -c FinalFilter.c -o FinalFilt\
> er.o
> FinalFilter.c:12: warning: unused parameter 'Data'
> FinalFilter.c:12: warning: unused parameter 'length'
>         gcc -Wall -W -Wwrite-strings -Wpointer-arith -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wnested-externs -Wconversion -Wundef -Winline -fno-dollars-in-identifiers  putarchive.c -o putarchive -D\
> put_main=main
> putarchive.c: In function 'PutFile':
> putarchive.c:87: warning: passing argument 3 of 'send' as unsigned due to prototype
> putarchive.c:105: warning: passing argument 3 of 'write' as unsigned due to prototype
>         gcc -Wall -W -Wwrite-strings -Wpointer-arith -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wnested-externs -Wconversion -Wundef -Winline -fno-dollars-in-identifiers  -c putarchive.c -o putarchive\
> .o
> putarchive.c: In function 'PutFile':
> putarchive.c:87: warning: passing argument 3 of 'send' as unsigned due to prototype
> putarchive.c:105: warning: passing argument 3 of 'write' as unsigned due to prototype
>         gcc -Wall -W -Wwrite-strings -Wpointer-arith -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wnested-externs -Wconversion -Wundef -Winline -fno-dollars-in-identifiers  -c privateip.c -o privateip.o
>         gcc GrubCClient.o ARCFile.o FinalFilter.o putarchive.o privateip.o -o GrubCClient