[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:

(1 comment)

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);
> 3) Use asprintf() ?
> 3) Use asprintf() ?
 > 4) Prove that buf is longer than any possible combination of the
 > header lines.
 > 
 > IIRC, the http headers looked like there would be no truncation.
 > But I did not do the exact math.
 > 
 > I tried to avoid not implementing printf like API. But it would
 > give a powerful API, and would be probably simple to implement with
 > asprintf() (or perhaps have a decent stack buffer, use snprintf and
 > if that fails retry with dynamically allocated buffer).
 > 
 > I would probably opt to #2 since there are already three cases
 > which would become trivial to fix if the helper is done.
 > 
 > Any other thoughts for this?

I think the possibility of truncation needs to be accounted for, especially since arbitrary HTTP headers can be passed into this function.

My favorite method for addressing this issue is #2, purely for the convenience. However, I would be okay if you went with #1 or #3 instead.


-- 
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: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Timo Teräs <timo.teras at iki.fi>
Gerrit-HasComments: Yes