Julien Pierre's strsclnt patch

Julien Pierre's strsclnt patch

[ekr-rietveld, 2015-10-10 14:18:59]

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c
File cmd/strsclnt/strsclnt.c (right):

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:185: "       -L [uri] . Enable full-duplex mode, and
specify a custom URI to POST to\n",
I don't think that each of these flags should set full duplex as a side effect.
These are useful tests even if you aren't in full duplex. I would instead have a
flag that enabled full duplex and a separate flag(s) that sets up the test
scenario.

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:517: PRBool send_buf(PRFileDesc* fd, SECItem* buf)
const SECItem* buf

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:559: PK11_GenerateRandom(name.chars,
sizeof(name.chars));
You don't need a union here, just do a cast.

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:565: char* uri = NULL;
Please don't put file-level globals in the middle of the file.

Also, please make them static.

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:571: unsigned int chunk_size = fixedChunkSize;
Please don't assign and then re-assign.

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:575: unsigned int hdrlen = sprintf(chunk_buf, "%X\n",
chunk_size);
Use PR_snprintf()

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:577: chunk_buf[hdrlen+chunk_size] = '\n';
Please write a pattern here so you can verify that the return is correct.

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:584: PRFileDesc *	ssl_sock	= (PRFileDesc *)a;
Spaces, not tabs. Also no need for this weird alignment.

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:585: PRBool sentFooter = PR_FALSE, sentChunks =
PR_FALSE;
Declare these on separate lines.

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:588: const char* last_chunk = "0\n\n";
\r\n

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:591: if (!fixedChunkSize) {
Use ?:

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:598: chunk_buffer_size += CHUNK_HEADER_FOOTER_SIZE;
If you're going to try to pack things into an SSL record, then you can't add the
footer to the maximum size.

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:600: char* chunk_buf = (char*)
malloc(chunk_buffer_size);
C malloc return values do not need to be casted.

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:605: }
Please don't assign this and then reassign. do the fixedNumChunks assignment in
the conditional.

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:606: int i = 0;
This line i superflous.

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:607: for (i=0;i<numChunks;i++) {
for (i = 0; i < numChunks; i++)

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:609: if (PR_FALSE == send_string(ssl_sock, chunk_buf) )
break;
if (!send_string(...)) { 
  // print an error here.
  break;
}

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:615: /* send footer . This is an HTTP request with a
Connection: close header,
Nit. No space between text and ".", throughout this comment and others.

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:620: const char* footer = "GET / HTTP/1.1\nHost:
localhost\nConnection: close\n\n\n";
I believe we are still requiring declarations to be at the top of the function.

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:620: const char* footer = "GET / HTTP/1.1\nHost:
localhost\nConnection: close\n\n\n";
You need a real hostname, not 'localhost'

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:624: if (sentChunks && sentFooter) {
Why are you sending the footer when a chunk failed.

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:640: to a server URI that will accept an indefinite
amount of POST data .
Nit: no space between data and '.'

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:645: 
This seems like the wrong semantic. You could get errors from the server prior
to writing the entire header. Why not instead do SSL_ForceHAndshake()

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:648: "POST %s HTTP/1.1\nHost: localhost\nContent-Type:
text/plain\n"
Use a real hostname, not "localhost"

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:649: "Connection: Keep-Alive\nTransfer-Encoding:
chunked\n\n";
HTTP headers use CRLF, not \n. This needs to be fixed globally.

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:658: }
Use ?: here

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:659: char* header = PR_smprintf(defaultHeader, thisUri);
This is a small, basically fixed size object. Just allocate it on the stack and
us PR_snprintf.

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:670: PRThread* writer =  PR_CreateThread(PR_USER_THREAD,
do_writes, (void*)ssl_sock,
You do not need to cast pointers to void *.

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:684: count = PR_Recv(ssl_sock, buf, RD_BUF_SIZE-1, 0,
maxInterval);
Can you explain why -1 here? Perhaps a comment.

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:699: FPRINTF(stderr, "%s", buf); /* print HTTP response
from server */
What's the point of printing the response?

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:700: } while (count>0);
while(count > 0)

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:706: PR_JoinThread(writer);
You don't seem to be checking that you are getting back the data that was put
in. I thought that was part of the point here.

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:720: /* half-duplex mode, single thread. Ie. write
request, then read response, sequentially */
Not: "I.e.,"

End this comment with a period.

https://codereview.appspot.com/271820043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:1368: bigBuf.data = PORT_Malloc(bigBuf.len);
You seem to have repurposed bigBuf to only hold a header. It should be renamed.