[Asterisk-code-review] Implement internal abstraction for iostreams (asterisk[master])

[Mark Michelson]

Mark Michelson has posted comments on this change. ( https://gerrit.asterisk.org/2932 )

Change subject: Implement internal abstraction for iostreams
......................................................................


Patch Set 5: Code-Review-1

(8 comments)

https://gerrit.asterisk.org/#/c/2932/5/apps/app_externalivr.c
File apps/app_externalivr.c:

PS5, Line 169: 	ast_str_append(&tmp, 0, "\n");
             : 	ast_iostream_write(stream, ast_str_buffer(tmp), strlen(ast_str_buffer(tmp)));
             : 
             : 	ast_debug(1, "sent '%s'", ast_str_buffer(tmp));
This edit has a sliiight problem to it. The debug message goes from looking like this:

    sent 'foo'

to

    sent 'foo
    '


https://gerrit.asterisk.org/#/c/2932/5/main/http.c
File main/http.c:

PS5, Line 510: 	len = snprintf(buf, sizeof(buf),
             : 		"HTTP/1.1 %d %s\r\n"
             : 		"%s"
             : 		"Date: %s\r\n"
             : 		"%s"
             : 		"%s"
             : 		"%s"
             : 		"Content-Length: %d\r\n"
             : 		"\r\n",
             : 		status_code, status_title ? status_title : "OK",
             : 		ast_str_buffer(server_header_field),
             : 		timebuf,
             : 		close_connection ? "Connection: close\r\n" : "",
             : 		static_content ? "" : "Cache-Control: no-cache, no-store\r\n",
             : 		http_header ? ast_str_buffer(http_header) : "",
             : 		content_length
             : 		);
             : 	ast_iostream_write(ser->stream, buf, len);
Two potential problems here:

1) The size limitation of buf could cause us to truncate output, resulting in an incomplete HTTP packet.
2) If output is truncated, the return value of snprintf is the number of bytes that would have been written if the output had not been truncated. This means that you could pass a larger size than desired to ast_iostream_write().

I can see two potential ways to solve this.

1) Fix the problem here by using an ast_str structure, which allows for the string to grow dynamically.
2) Create an ast_iostream_write() variant that allows printf-style parameters to be sent to it.


PS5, Line 1060: 	res = ast_iostream_read(ser->stream, chunk_sync, sizeof(chunk_sync));
              : 	if (res < sizeof(chunk_sync)) {
              : 		ast_log(LOG_WARNING, "Short HTTP chunk sync read (Wanted %zu)\n",
              : 			sizeof(chunk_sync));
              : 		return -1;
              : 	}
The if logic has been altered here. This may actually be correct with the change, but it stood out as being different.


https://gerrit.asterisk.org/#/c/2932/5/main/iostream.c
File main/iostream.c:

PS5, Line 257: 		r = iostream_read(stream, stream->rbufhead, sizeof(stream->rbuf) - stream->rbuflen);
             : 		if (r <= 0) {
             : 			return r;
             : 		}
Should the second argument to iostream_read() be

   stream->rbufhead + stream->rbuflen

?

Otherwise, you would be overwriting what is already in the read buffer?


PS5, Line 455: static void iostream_dtor(void *cookie)
             : {
             : #ifdef AST_DEVMODE
             : 	/* Since the ast_assert below is the only one using stream,
             : 	 * and ast_assert is only available with AST_DEVMODE, we
             : 	 * put this in a conditional to avoid compiler warnings. */
             : 	struct ast_iostream *stream = cookie;
             : #endif
             : 
             : 	ast_assert(stream->fd == -1);
             : }
Does this compile when not using AST_DEVMODE? I know that the ast_assert macro is defined as nothing when not using AST_DEVMODE. Does C evaluates the arguments to the macro, though? Because if so, "stream" is not declared if you are not using AST_DEVMODE.


Line 509: 	} 
Extra trailing space on this line.


https://gerrit.asterisk.org/#/c/2932/5/res/res_http_websocket.c
File res/res_http_websocket.c:

PS5, Line 851: 		if (protocol) {
             : 			len = snprintf(buf, sizeof(buf), "HTTP/1.1 101 Switching Protocols\r\n"
             : 				"Upgrade: %s\r\n"
             : 				"Connection: Upgrade\r\n"
             : 				"Sec-WebSocket-Accept: %s\r\n"
             : 				"Sec-WebSocket-Protocol: %s\r\n\r\n",
             : 				upgrade,
             : 				websocket_combine_key(key, base64, sizeof(base64)),
             : 				protocol);
             : 		} else {
             : 			len = snprintf(buf, sizeof(buf), "HTTP/1.1 101 Switching Protocols\r\n"
             : 				"Upgrade: %s\r\n"
             : 				"Connection: Upgrade\r\n"
             : 				"Sec-WebSocket-Accept: %s\r\n\r\n",
             : 				upgrade,
             : 				websocket_combine_key(key, base64, sizeof(base64)));
             : 		}
This has the same problem as the other place where we were writing HTTP to a buffer. This has the possibility of truncation.


PS5, Line 1311: 	len = snprintf(buf, sizeof(buf),
              : 		 "GET /%s HTTP/1.1\r\n"
              : 		 "Sec-WebSocket-Version: %d\r\n"
              : 		 "Upgrade: websocket\r\n"
              : 		 "Connection: Upgrade\r\n"
              : 		 "Host: %s\r\n"
              : 		 "Sec-WebSocket-Key: %s\r\n"
              : 		 "%s\r\n",
              : 		 client->resource_name ? ast_str_buffer(client->resource_name) : "",
              : 		 client->version,
              : 		 client->host,
              : 		 client->key,
              : 		 protocols);
Another possible truncation point.


-- 
To view, visit https://gerrit.asterisk.org/2932
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id916aef418b665ced6a7489aef74908b6e376e85
Gerrit-PatchSet: 5
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Timo Teräs <timo.teras at iki.fi>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Timo Teräs <timo.teras at iki.fi>
Gerrit-HasComments: Yes