[Grub-dev] GrubCClient review [Was: Re: Yet another invalid workunit entry]
Balinny
balinny at gmail.com
Fri Jul 11 21:45:19 UTC 2008
Rainer Blome wrote:
> 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?
>
No problems detected by valgrind ;)
>>> 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?
>
Well, they're Grub New Generation :-)
Every client is new code.
>>> 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.
>
Of course!
> o "return -1" statements leak one socket each (the socket
> is not closed).
>
Nope. The two return -1 on FetchPage are before the socket is created.
However, PutFile _could_
leak a socket. I need to review Putarchive
> 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.
>
Changed to C-style comments.
> 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.
>
In a word, they don't support C99
> Run GCC with more warning options, in particular -W.
> See below for a suggested set of warning options and gcc's output.
>
I use -Wall precisely to avoid specifying a gazillion options... But
heh, some of these are useful.
I spent some time trying to figure out what was -W (pass a warning, but
without name?) until if
found on the man that it's an old name of -Wextra
> 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;
>
>
> putarchive.c:51: char* HostPortSplit = strchr(Host, ':');
> GrubCClient.c:88: char* HostPortSplit = strchr(Server, ':');
> GrubCClient.c:152: size_t QueryLen = strlen(Server) +
> strlen(Path) + strlen(UserAgent) + sizeof(Petition) - 3*2;
>
I could limit them by MAXHOSTNAMELEN, MAXURLLEN, MAXUSERAGENTLEN...
But the function shouldn't need to know that values. Just get proper
nul-ended strings.
> GrubCClient.c:288: if (!(ContentType = MIME_Verify((signed
> char*)strtok(ContentType, " ;"))))
> GrubCClient.c:298: size_t sLen = strlen(LineBuffer) + 1;
>
That part needs to be refactored. Too much interdependence.
> 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:56:char* chrntok(char* str, size_t n, char 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;
>
Your regex doesn't like my new function...
> 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]);
>
All of these are innocent strings :)
> 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);
>
These might benfefit of that.
> GrubCClient.c:518: char * Y = strdup(C + sizeof(CompressCmd) - 1);
>
Note that on windows there's no strndup()
> GrubCClient.c:589: hlen = strlen(headername);
>
Hardcoded by the program. So safe to use.
> putarchive.c:102: strcpy(Buffer+9, "XXX");
>
I'll need be careful not to overflow those 1015 bytes with three X :-)
>
> # ----------------------------------------------------------------------
> # 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
>>
This is due to using -Wundef
If it's undefined it'd produce the error, so probably not needed.
>> GrubCClient.c:53: warning: function declaration isn't a prototype
>> GrubCClient.c:71: warning: function declaration isn't a prototype
>>
Fixed.
>> 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: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'
>>
_Should_ be fixed. %zu on *nix, %I on Windows.
>> GrubCClient.c:164: warning: comparison between signed and unsigned
>>
Casted.
>> GrubCClient.c:239: warning: comparison between signed and unsigned
>> GrubCClient.c:248: warning: comparison between signed and unsigned
>>
Fixed.
>> GrubCClient.c:264: warning: passing argument 3 of 'recv' as unsigned due to prototype
>>
Changed. But take into account that send() and recv() have the length
parameter as size_t on POSIX. However,
Windows defines them as int :(
>> GrubCClient.c:286: warning: assignment discards qualifiers from pointer target type
>> GrubCClient.c:289: warning: assignment discards qualifiers from pointer target type
>>
This is a tricky one. ContentType is either a modifiable string from the
page, or a constant. We can't set it as a const pointer
because strtok() would get passed a cont. The code does the right thing
but the warning doesn't know. Could add another cast
to silent it but don't seem good. Probably better to move to another
function.
>> 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'
>>
See above.
>> GrubCClient.c:304: warning: passing argument 2 of 'ARCWrite' as signed due to prototype
>> GrubCClient.c:322: warning: passing argument 2 of 'ARCWrite' as signed due to prototype
>>
Fixed.
>> GrubCClient.c:309: warning: passing argument 3 of 'chrntok' with different width due to prototype
>>
So, where's the problem? I say it gets a character as parameter, you
can't place a single byte on the stack and it's promoted to int.
>> GrubCClient.c: In function 'chrntok':
>> GrubCClient.c:333: warning: comparison between signed and unsigned
>>
Fixed.
>> GrubCClient.c: In function 'copytoEOL':
>> GrubCClient.c:353: warning: comparison between signed and unsigned
>>
Fixed
>> GrubCClient.c:526:6: warning: "DELAY_UPLOAD" is not defined
>> GrubCClient.c:537:18: warning: "KEEP_FILES" is not defined
>>
They're designed to work if 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
>>
Fixed.
>> 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
>>
Fixed.
>> 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'
>>
That's because that function is a stub. It'd probably shuts up if i skip
the parameter names, but i don't like that option too much...
>> 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
>>
I don't mind! they won't be negative!! Some warnings are quite annoying :)
More information about the Grub-dev
mailing list