implement http server/client#1
Conversation
takumihara
left a comment
takumihara
left a comment
There was a problem hiding this comment.
お疲れ様です！goto cleanup/fatal でリソース解放をまとめているのが良いですね。strtol を使った数式パースで連続演算（4+5-3+1）に対応しているのも面白いです！
| goto fatal; | ||
| } | ||
|
|
||
| int n = read(conn_fd, buffer, BUFFERSIZE); |
There was a problem hiding this comment.
read は1回の呼び出しで全データが読めるとは限らなくて、ネットワーク越しだとリクエストが分割されて届くことがあります。\r\n\r\n（ヘッダの終端）が見つかるまでループで読み続けるようにすると安全です！
あと、read(conn_fd, buffer, BUFFERSIZE) だと、ちょうど1024バイト読んだ場合に NUL 終端がなくなって、後続の strtok や strcpy がバッファの外を読んでしまいます。BUFFERSIZE - 1 にすると安全です。
細かいですが、read の返り値の型は ssize_t です！
https://man7.org/linux/man-pages/man2/read.2.html
| goto cleanup; | ||
| } | ||
|
|
||
| if (write(conn_fd, result, strlen(result)) == -1) { |
| goto cleanup; | ||
| } | ||
|
|
||
| if (write(conn_fd, result, strlen(result)) == -1) { |
There was a problem hiding this comment.
SIGPIPE のハンドリングも入れてもいいかもです！クライアントが途中で切断した後に write すると SIGPIPE でプロセスが死んじゃいます
| if (strcmp(version, "HTTP/1.1\r\n\r\n") != 0) { | ||
| fprintf(stderr, "Unsupported HTTP version: %s (only HTTP/1.1 is supported)\n", version); | ||
| } |
There was a problem hiding this comment.
ここ、fprintf だけで return していないので、バージョンが不正でも処理が続行しちゃいますね。
あと、ブラウザからのリクエストだとヘッダが付くので、version のトークンに \r\n\r\n が含まれないケースがあって、その場合は常に不一致になりそうです
| fprintf(stderr, "HTTP header error"); | ||
| return EXIT_FAILURE; | ||
| } | ||
| strcpy(method, token); |
There was a problem hiding this comment.
こういうリクエストが来たらどうなりますか？
AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA /calc?query=1+2 HTTP/1.1
method[16] に strcpy で長さ制限なしで書き込んでいるので、スタックが壊れそうです
| if (strtok(NULL, " ") != NULL) { | ||
| fprintf(stderr, "HTTP header error"); | ||
| return EXIT_FAILURE; | ||
| } |
There was a problem hiding this comment.
ブラウザからリクエストを送ると、ヘッダが付きますよね？
GET /calc?query=1+2 HTTP/1.1\r\nHost: localhost\r\n\r\n
スペースで strtok すると4つ以上のトークンが出て、ここでエラーになりませんか？
| if (parse_query(buffer, &value) != 0) { | ||
| goto cleanup; |
There was a problem hiding this comment.
パースが失敗したとき、クライアントには何もレスポンスを返さずに接続を切っていますが、400 Bad Request とかを返してあげるとクライアント側でエラーの原因が分かりやすくなりそうです
| return parse_formula(formula, strlen(formula), out); | ||
| } | ||
|
|
||
| int parse_formula(char* formula, const int formula_len, int* out) { |
|
|
||
| int parse_formula(char* formula, const int formula_len, int* out) { | ||
| char *p = formula; | ||
| int value = strtol(p, &p, 10); |
There was a problem hiding this comment.
strtol を使っているのはいいですね！ただ、例えば query=99999999999+1 みたいにめちゃくちゃ大きい数が来た場合、strtol は ERANGE を errno にセットするので、それをチェックするとオーバーフローを検出できます
2 participants
No description provided.