On 02/12/2013 03:14 PM, Michal Toman wrote:
>> + safe_waitpid(xz_child, &status, 0);
>> + if (!WIFEXITED(status) || WEXITSTATUS(status) != 0)
>> + {
>> + error_msg_and_die(_("Can't create temporary file in /tmp"));
>> + }
> ^^^ I can see some inconsistency here - on many places you put
> the curly braces around a single function and on many you don't.
I usually go about it like this:
in the beginning:
if (expr)
something;
later needed to add stuff:
if (expr)
{
something1;
something2;
}
later had to remove stuff:
if (expr)
{
something2;
}
^^^^^^^^^^^^ I don't bother removing {},
what if next change will need to add them again?
In this case I copy-pasted the ifs from another C file,
they already had {}s and I didn't remove them,
as per above policy of "don't remove {}s is they exist".
Superfluous {}s would be less painful if we'd had {
on the "if" line... <wink> <wink>
>> + err:
>> + alert_server_error(NULL);
>> + /* Show bad header to the user */
>> + char *sanitized = sanitize_utf8(message, (SANITIZE_ALL &
>> ~SANITIZE_TAB));
>
> Do we really need to sanitize? IIRC HTTP headers are never UTF-8.
> Either they are valid (ASCII) or totally broken and should probably not be
> displayed.
I am doing this not so much because of UTF-8, but because I don't want
to end up with a message like this:
Malformed HTTP response header: 'HTTP/z garbage
'"
i.e. with newline inside ''! sanitize_utf8() would show it in hex.
>> -void alert_server_error();
>> -void alert_connection_error();
>> +void alert_server_error(const char *peer_name);
>> +void alert_connection_error(const char *peer_name);
> You use 'peer_name' in the header and 'name' in the implementation.
Yes :)
Please take a look at patch v2:
diff -x '*.po' -d -urpN abrt.4/src/plugins/abrt-dedup-client.c
abrt.5/src/plugins/abrt-dedup-client.c
--- abrt.4/src/plugins/abrt-dedup-client.c 2013-02-07 12:13:52.000000000
+0100
+++ abrt.5/src/plugins/abrt-dedup-client.c 2013-02-11 19:28:13.441262441
+0100
@@ -198,8 +198,7 @@ int main(int argc, char **argv)
0/*flags*/, PR_INTERVAL_NO_TIMEOUT);
if (written < 0)
{
- PR_Close(ssl_sock);
- alert_connection_error();
+ alert_connection_error(cfg.url);
error_msg_and_die(_("Failed to send HTTP request of length %d: NSS
error %d."),
request->len, PR_GetError());
}
diff -x '*.po' -d -urpN abrt.4/src/plugins/abrt-retrace-client.c
abrt.5/src/plugins/abrt-retrace-client.c
--- abrt.4/src/plugins/abrt-retrace-client.c 2013-02-07 16:44:13.135211195
+0100
+++ abrt.5/src/plugins/abrt-retrace-client.c 2013-02-12 17:40:44.711793079
+0100
@@ -97,7 +97,7 @@ static int create_archive(bool unlink_te
char *filename = xstrdup("/tmp/abrt-retrace-client-archive-XXXXXX.tar.xz");
int tempfd = mkstemps(filename, /*suffixlen:*/7);
if (tempfd == -1)
- perror_msg_and_die(_("Cannot open temporary file"));
+ perror_msg_and_die(_("Can't create temporary file in /tmp"));
if (unlink_temp)
xunlink(filename);
free(filename);
@@ -114,6 +114,7 @@ static int create_archive(bool unlink_te
int tar_xz_pipe[2];
xpipe(tar_xz_pipe);
+
fflush(NULL); /* paranoia */
pid_t xz_child = vfork();
if (xz_child == -1)
@@ -167,13 +168,21 @@ static int create_archive(bool unlink_te
free((void*)tar_args[2]);
close(tar_xz_pipe[1]);
- /* Wait for tar and xz to finish */
+ /* Wait for tar and xz to finish successfully */
+ int status;
VERB1 log_msg("Waiting for tar...");
- safe_waitpid(tar_child, NULL, 0);
+ safe_waitpid(tar_child, &status, 0);
+ if (!WIFEXITED(status) || WEXITSTATUS(status) != 0)
+ /* Hopefully, by this time child emitted more meaningful
+ * error message. But just in case it didn't:
+ */
+ error_msg_and_die(_("Can't create temporary file in /tmp"));
VERB1 log_msg("Waiting for xz...");
- safe_waitpid(xz_child, NULL, 0);
-//FIXME: need to check that both exited with 0! Think about out-of-disk-space
situation...
+ safe_waitpid(xz_child, &status, 0);
+ if (!WIFEXITED(status) || WEXITSTATUS(status) != 0)
+ error_msg_and_die(_("Can't create temporary file in /tmp"));
VERB1 log_msg("Done...");
+
xlseek(tempfd, 0, SEEK_SET);
return tempfd;
}
@@ -195,9 +204,8 @@ struct retrace_settings *get_settings()
/*flags:*/0, PR_INTERVAL_NO_TIMEOUT);
if (written == -1)
{
- PR_Close(ssl_sock);
- alert_connection_error();
- error_msg_and_die(_("Failed to send HTTP header of length %d: NSS
error %d."),
+ alert_connection_error(cfg.url);
+ error_msg_and_die(_("Failed to send HTTP header of length %d: NSS
error %d"),
http_request->len, PR_GetError());
}
strbuf_free(http_request);
@@ -208,7 +216,7 @@ struct retrace_settings *get_settings()
int response_code = http_get_response_code(http_response);
if (response_code != 200)
{
- alert_server_error();
+ alert_server_error(cfg.url);
error_msg_and_die(_("Unexpected HTTP response from server: %d\n%s"),
response_code, http_response);
}
@@ -217,7 +225,7 @@ struct retrace_settings *get_settings()
char *c, *row, *value;
if (!headers_end)
{
- alert_server_error();
+ alert_server_error(cfg.url);
error_msg_and_die(_("Invalid response from server: missing HTTP
message body."));
}
row = headers_end + strlen("\r\n\r\n");
@@ -363,8 +371,7 @@ static int check_package(const char *nvr
/*flags:*/0, PR_INTERVAL_NO_TIMEOUT);
if (written == -1)
{
- PR_Close(ssl_sock);
- alert_connection_error();
+ alert_connection_error(cfg.url);
error_msg_and_die(_("Failed to send HTTP header of length %d: NSS
error %d"),
http_request->len, PR_GetError());
}
@@ -378,7 +385,7 @@ static int check_package(const char *nvr
if (response_code != 302 && response_code != 404)
{
char *http_body = http_get_body(http_response);
- alert_server_error();
+ alert_server_error(cfg.url);
error_msg_and_die(_("Unexpected HTTP response from server: %d\n%s"),
response_code, http_body);
}
@@ -426,20 +433,16 @@ static int create(bool delete_temp_archi
/* get raw size */
if (coredump)
{
- if (stat(coredump, &file_stat) == -1)
- error_msg_and_die(_("Unable to stat file '%s'."), coredump);
-
+ xstat(coredump, &file_stat);
unpacked_size = (long long)file_stat.st_size;
}
else if (dump_dir_name != NULL)
{
struct dump_dir *dd = dd_opendir(dump_dir_name, /*flags*/ 0);
if (!dd)
- error_msg_and_die(_("Unable to open dump directory '%s'."),
dump_dir_name);
-
+ xfunc_die(); /* dd_opendir already emitted error message */
if (dd_exist(dd, FILENAME_VMCORE))
task_type = TASK_VMCORE;
-
dd_close(dd);
char *path;
@@ -448,13 +451,7 @@ static int create(bool delete_temp_archi
while (required_files[i])
{
path = concat_path_file(dump_dir_name, required_files[i]);
- if (stat(path, &file_stat) == -1)
- {
- error_msg(_("Unable to stat file '%s'."), path);
- free(path);
- xfunc_die();
- }
-
+ xstat(path, &file_stat);
free(path);
if (!S_ISREG(file_stat.st_mode))
@@ -507,7 +504,7 @@ static int create(bool delete_temp_archi
if (!supported)
{
- alert_server_error();
+ alert_server_error(cfg.url);
error_msg_and_die(_("The server does not support "
"xz-compressed tarballs."));
}
@@ -652,9 +649,8 @@ static int create(bool delete_temp_archi
/*flags:*/0, PR_INTERVAL_NO_TIMEOUT);
if (written == -1)
{
- PR_Close(ssl_sock);
- alert_connection_error();
- error_msg_and_die(_("Failed to send HTTP header of length %d: NSS
error %d."),
+ alert_connection_error(cfg.url);
+ error_msg_and_die(_("Failed to send HTTP header of length %d: NSS
error %d"),
http_request->len, PR_GetError());
}
@@ -711,7 +707,7 @@ static int create(bool delete_temp_archi
if the server send some explanation regarding the
error. */
result = 1;
- alert_connection_error();
+ alert_connection_error(cfg.url);
error_msg(_("Failed to send data: NSS error %d (%s): %s"),
PR_GetError(),
PR_ErrorToName(PR_GetError()),
@@ -732,7 +728,7 @@ static int create(bool delete_temp_archi
char *http_body = http_get_body(http_response);
if (!http_body)
{
- alert_server_error();
+ alert_server_error(cfg.url);
error_msg_and_die(_("Invalid response from server: missing HTTP
message body."));
}
if (http_show_headers)
@@ -740,7 +736,7 @@ static int create(bool delete_temp_archi
int response_code = http_get_response_code(http_response);
if (response_code == 500 || response_code == 507)
{
- alert_server_error();
+ alert_server_error(cfg.url);
error_msg_and_die(http_body);
}
else if (response_code == 403)
@@ -752,20 +748,20 @@ static int create(bool delete_temp_archi
}
else if (response_code != 201)
{
- alert_server_error();
+ alert_server_error(cfg.url);
error_msg_and_die(_("Unexpected HTTP response from server: %d\n%s"),
response_code, http_body);
}
free(http_body);
*task_id = http_get_header_value(http_response, "X-Task-Id");
if (!*task_id)
{
- alert_server_error();
+ alert_server_error(cfg.url);
error_msg_and_die(_("Invalid response from server: missing
X-Task-Id."));
}
*task_password = http_get_header_value(http_response, "X-Task-Password");
if (!*task_password)
{
- alert_server_error();
+ alert_server_error(cfg.url);
error_msg_and_die(_("Invalid response from server: missing
X-Task-Password."));
}
free(http_response);
@@ -830,8 +826,7 @@ static void status(const char *task_id,
/*flags:*/0, PR_INTERVAL_NO_TIMEOUT);
if (written == -1)
{
- PR_Close(ssl_sock);
- alert_connection_error();
+ alert_connection_error(cfg.url);
error_msg_and_die(_("Failed to send HTTP header of length %d: NSS
error %d"),
http_request->len, PR_GetError());
}
@@ -840,7 +835,7 @@ static void status(const char *task_id,
char *http_body = http_get_body(http_response);
if (!*http_body)
{
- alert_server_error();
+ alert_server_error(cfg.url);
error_msg_and_die(_("Invalid response from server: missing HTTP
message body."));
}
if (http_show_headers)
@@ -848,14 +843,14 @@ static void status(const char *task_id,
int response_code = http_get_response_code(http_response);
if (response_code != 200)
{
- alert_server_error();
+ alert_server_error(cfg.url);
error_msg_and_die(_("Unexpected HTTP response from server: %d\n%s"),
response_code, http_body);
}
*task_status = http_get_header_value(http_response, "X-Task-Status");
if (!*task_status)
{
- alert_server_error();
+ alert_server_error(cfg.url);
error_msg_and_die(_("Invalid response from server: missing
X-Task-Status."));
}
*status_message = http_body;
@@ -909,8 +904,7 @@ static void backtrace(const char *task_i
/*flags:*/0, PR_INTERVAL_NO_TIMEOUT);
if (written == -1)
{
- PR_Close(ssl_sock);
- alert_connection_error();
+ alert_connection_error(cfg.url);
error_msg_and_die(_("Failed to send HTTP header of length %d: NSS
error %d."),
http_request->len, PR_GetError());
}
@@ -919,7 +913,7 @@ static void backtrace(const char *task_i
char *http_body = http_get_body(http_response);
if (!http_body)
{
- alert_server_error();
+ alert_server_error(cfg.url);
error_msg_and_die(_("Invalid response from server: missing HTTP
message body."));
}
if (http_show_headers)
@@ -927,7 +921,7 @@ static void backtrace(const char *task_i
int response_code = http_get_response_code(http_response);
if (response_code != 200)
{
- alert_server_error();
+ alert_server_error(cfg.url);
error_msg_and_die(_("Unexpected HTTP response from server: %d\n%s"),
response_code, http_body);
}
@@ -978,8 +972,7 @@ static void run_log(const char *task_id,
/*flags:*/0, PR_INTERVAL_NO_TIMEOUT);
if (written == -1)
{
- PR_Close(ssl_sock);
- alert_connection_error();
+ alert_connection_error(cfg.url);
error_msg_and_die(_("Failed to send HTTP header of length %d: NSS
error %d."),
http_request->len, PR_GetError());
}
@@ -988,7 +981,7 @@ static void run_log(const char *task_id,
char *http_body = http_get_body(http_response);
if (!http_body)
{
- alert_server_error();
+ alert_server_error(cfg.url);
error_msg_and_die(_("Invalid response from server: missing HTTP
message body."));
}
if (http_show_headers)
@@ -996,7 +989,7 @@ static void run_log(const char *task_id,
int response_code = http_get_response_code(http_response);
if (response_code != 200)
{
- alert_server_error();
+ alert_server_error(cfg.url);
error_msg_and_die(_("Unexpected HTTP response from server: %d\n%s"),
response_code, http_body);
}
diff -x '*.po' -d -urpN abrt.4/src/plugins/https-utils.c
abrt.5/src/plugins/https-utils.c
--- abrt.4/src/plugins/https-utils.c 2013-02-07 12:13:52.000000000 +0100
+++ abrt.5/src/plugins/https-utils.c 2013-02-12 17:41:54.958793469 +0100
@@ -41,15 +41,28 @@ void get_language(struct language *lang)
++lang->encoding;
}
-void alert_server_error()
+void alert_server_error(const char *peer_name)
{
- alert(_("An error occurred on the server side. Try again later."));
+ if (!peer_name)
+ alert(_("An error occurred on the server side."));
+ else
+ {
+ char *msg = xasprintf(_("A server-side error occurred on '%s'"),
peer_name);
+ alert(msg);
+ free(msg);
+ }
}
-void alert_connection_error()
+void alert_connection_error(const char *peer_name)
{
- alert(_("An error occurred while connecting to the server. "
- "Check your network connection and try again."));
+ if (!peer_name)
+ alert(_("An error occurred while connecting to the server"));
+ else
+ {
+ char *msg = xasprintf(_("An error occurred while connecting to '%s'"),
peer_name);
+ alert(msg);
+ free(msg);
+ }
}
static SECStatus ssl_bad_cert_handler(void *arg, PRFileDesc *sock)
@@ -150,16 +163,18 @@ void ssl_connect(struct https_cfg *cfg,
PRAddrInfo *addrinfo = PR_GetAddrInfoByName(cfg->url, PR_AF_UNSPEC,
PR_AI_ADDRCONFIG);
if (!addrinfo)
{
- alert_connection_error();
- error_msg_and_die(_("Failed to get host by name: NSS error %d."),
PR_GetError());
+ alert_connection_error(cfg->url);
+ error_msg_and_die(_("Can't resolve host name '%s'. NSS error %d."),
cfg->url, PR_GetError());
}
+ /* Hack */
+ ssl_allow_insecure = cfg->ssl_allow_insecure;
+
void *enumptr = NULL;
PRNetAddr addr;
*tcp_sock = NULL;
- ssl_allow_insecure = cfg->ssl_allow_insecure;
- while ((enumptr = PR_EnumerateAddrInfo(enumptr, addrinfo, cfg->port,
&addr)))
+ while ((enumptr = PR_EnumerateAddrInfo(enumptr, addrinfo, cfg->port,
&addr)) != NULL)
{
if (addr.raw.family == PR_AF_INET || addr.raw.family == PR_AF_INET6)
{
@@ -167,80 +182,64 @@ void ssl_connect(struct https_cfg *cfg,
break;
}
}
-
PR_FreeAddrInfo(addrinfo);
-
if (!*tcp_sock)
- error_msg_and_die(_("Failed to create a TCP socket"));
+ /* Host exists, but has neither IPv4 nor IPv6?? */
+ error_msg_and_die(_("Can't resolve host name '%s'."), cfg->url);
+
+ /* These operations are expected to always succeed: */
PRSocketOptionData sock_option;
- sock_option.option = PR_SockOpt_Nonblocking;
+ sock_option.option = PR_SockOpt_Nonblocking;
sock_option.value.non_blocking = PR_FALSE;
- PRStatus pr_status = PR_SetSocketOption(*tcp_sock, &sock_option);
- if (PR_SUCCESS != pr_status)
- {
- PR_Close(*tcp_sock);
+ if (PR_SUCCESS != PR_SetSocketOption(*tcp_sock, &sock_option))
error_msg_and_die(_("Failed to set socket blocking mode."));
- }
*ssl_sock = SSL_ImportFD(NULL, *tcp_sock);
if (!*ssl_sock)
- {
- PR_Close(*tcp_sock);
error_msg_and_die(_("Failed to wrap TCP socket by SSL."));
- }
- SECStatus sec_status = SSL_OptionSet(*ssl_sock, SSL_HANDSHAKE_AS_CLIENT,
PR_TRUE);
- if (SECSuccess != sec_status)
- {
- PR_Close(*ssl_sock);
+ if (SECSuccess != SSL_OptionSet(*ssl_sock, SSL_HANDSHAKE_AS_CLIENT,
PR_TRUE))
error_msg_and_die(_("Failed to enable client handshake to SSL
socket."));
- }
-
if (SECSuccess != SSL_OptionSet(*ssl_sock, SSL_ENABLE_SSL2, PR_TRUE))
error_msg_and_die(_("Failed to enable client handshake to SSL
socket."));
if (SECSuccess != SSL_OptionSet(*ssl_sock, SSL_ENABLE_SSL3, PR_TRUE))
error_msg_and_die(_("Failed to enable client handshake to SSL
socket."));
if (SECSuccess != SSL_OptionSet(*ssl_sock, SSL_ENABLE_TLS, PR_TRUE))
error_msg_and_die(_("Failed to enable client handshake to SSL
socket."));
-
- sec_status = SSL_SetURL(*ssl_sock, cfg->url);
- if (SECSuccess != sec_status)
- {
- PR_Close(*ssl_sock);
+ if (SECSuccess != SSL_SetURL(*ssl_sock, cfg->url))
error_msg_and_die(_("Failed to set URL to SSL socket."));
- }
- pr_status = PR_Connect(*ssl_sock, &addr, PR_INTERVAL_NO_TIMEOUT);
- if (PR_SUCCESS != pr_status)
+
+ /* This finally sends packets down the wire.
+ * If we fail here, then server denied our connect, or is down, etc.
+ * Need a good error message.
+ */
+ if (PR_SUCCESS != PR_Connect(*ssl_sock, &addr, PR_INTERVAL_NO_TIMEOUT))
{
- PR_Close(*ssl_sock);
- alert_connection_error();
- error_msg_and_die(_("Failed to connect SSL address."));
+ alert_connection_error(cfg->url);
+ error_msg_and_die(_("Can't connect to '%s'"), cfg->url);
}
+
+ /* These should not fail either. (Why we don't set them earlier?) */
if (SECSuccess != SSL_BadCertHook(*ssl_sock,
(SSLBadCertHandler)ssl_bad_cert_handler,
NULL))
{
- PR_Close(*ssl_sock);
error_msg_and_die(_("Failed to set certificate hook."));
}
if (SECSuccess != SSL_HandshakeCallback(*ssl_sock,
(SSLHandshakeCallback)ssl_handshake_callback,
NULL))
{
- PR_Close(*ssl_sock);
error_msg_and_die(_("Failed to set handshake callback."));
}
- sec_status = SSL_ResetHandshake(*ssl_sock, /*asServer:*/PR_FALSE);
- if (SECSuccess != sec_status)
+ if (SECSuccess != SSL_ResetHandshake(*ssl_sock, /*asServer:*/PR_FALSE))
{
- PR_Close(*ssl_sock);
- alert_server_error();
error_msg_and_die(_("Failed to reset handshake."));
}
- sec_status = SSL_ForceHandshake(*ssl_sock);
- if (SECSuccess != sec_status)
+
+ /* This performs SSL/TLS negotiation */
+ if (SECSuccess != SSL_ForceHandshake(*ssl_sock))
{
- PR_Close(*ssl_sock);
- alert_server_error();
- error_msg_and_die(_("Failed to force handshake: NSS error %d."),
+ alert_connection_error(cfg->url);
+ error_msg_and_die(_("Failed to complete SSL handshake: NSS error %d."),
PR_GetError());
}
}
@@ -301,23 +300,20 @@ char *http_get_body(const char *message)
int http_get_response_code(const char *message)
{
if (0 != strncmp(message, "HTTP/", strlen("HTTP/")))
- {
- alert_server_error();
- error_msg_and_die(_("Invalid response from server: HTTP header not
found."));
- }
- char *space = strstr(message, " ");
+ goto err;
+ char *space = strchr(message, ' ');
if (!space)
- {
- alert_server_error();
- error_msg_and_die(_("Invalid response from server: HTTP header not
found."));
- }
+ goto err;
int response_code;
if (1 != sscanf(space + 1, "%d", &response_code))
- {
- alert_server_error();
- error_msg_and_die(_("Invalid response from server: HTTP header not
found."));
- }
+ goto err;
return response_code;
+
+ err:
+ alert_server_error(NULL);
+ /* Show bad header to the user */
+ char *sanitized = sanitize_utf8(message, (SANITIZE_ALL & ~SANITIZE_TAB));
+ error_msg_and_die(_("Malformed HTTP response header: '%s'"), sanitized ?
sanitized : message);
}
void http_print_headers(FILE *file, const char *message)
@@ -353,7 +349,7 @@ char *tcp_read_response(PRFileDesc *tcp_
}
if (received == -1)
{
- alert_connection_error();
+ alert_connection_error(NULL);
error_msg_and_die(_("Receiving of data failed: NSS error %d."),
PR_GetError());
}
diff -x '*.po' -d -urpN abrt.4/src/plugins/https-utils.h
abrt.5/src/plugins/https-utils.h
--- abrt.4/src/plugins/https-utils.h 2013-02-07 12:13:52.000000000 +0100
+++ abrt.5/src/plugins/https-utils.h 2013-02-11 18:00:59.896317431 +0100
@@ -49,8 +49,8 @@ struct https_cfg
};
void get_language(struct language *lang);
-void alert_server_error();
-void alert_connection_error();
+void alert_server_error(const char *peer_name);
+void alert_connection_error(const char *peer_name);
void ssl_connect(struct https_cfg *cfg, PRFileDesc **tcp_sock, PRFileDesc
**ssl_sock);
void ssl_disconnect(PRFileDesc *ssl_sock);
char *http_get_header_value(const char *message, const char *header_name);
