Two patches: 01 splits HTTPS util funcs to a separate file. 02 adds the dedup-client.
You can test the dedup client on the 10.34.24.254 machine (I'm not sure how the public one works, there are some issues with older python in RHEL6) abrt-dedup-client <component> <backtrace_file> -k --url=10.34.24.254 Example: abrt-dedup-client coreutils ~/crashes/sleep/backtrace -k --url=10.34.24.254
Run-tested, please review.
Michal
Michal Toman mtoman@redhat.com writes:
Two patches: 01 splits HTTPS util funcs to a separate file. 02 adds the dedup-client.
You can test the dedup client on the 10.34.24.254 machine (I'm not sure how the public one works, there are some issues with older python in RHEL6) abrt-dedup-client <component> <backtrace_file> -k --url=10.34.24.254 Example: abrt-dedup-client coreutils ~/crashes/sleep/backtrace -k --url=10.34.24.254
Run-tested, please review.
Michal
static const char *dump_dir_name = NULL; static const char *coredump = NULL; -static const char *url = "retrace.fedoraproject.org"; static const char *required_retrace[] = { FILENAME_COREDUMP, FILENAME_EXECUTABLE, FILENAME_PACKAGE, @@ -66,23 +56,9 @@ static const char *required_retrace[] = { FILENAME_COREDUMP, NULL }; static const char *required_vmcore[] = { FILENAME_VMCORE, NULL }; -static bool ssl_allow_insecure = false; -static bool http_show_headers = false; -static unsigned port = 443; static unsigned delay = 0; static int task_type = TASK_RETRACE;
-#define PK11_SETATTRS(x,id,v,l) (x)->type = (id); \
- (x)->pValue=(v); (x)->ulValueLen = (l)
please consider to use 'do { ... } while(0)' in macro
#define PK11_SETATTRS(x,id,v,l) \ do { \ (x)->type = (id); \ (x)->pValue = (v); \ (x)->ulValueLen = (l) \ } while(0)
see http://kernelnewbies.org/FAQ/DoWhile0
diff --git a/src/plugins/https-utils.c b/src/plugins/https-utils.c index e69de29..e5ed7b5 100644 --- a/src/plugins/https-utils.c +++ b/src/plugins/https-utils.c @@ -0,0 +1,360 @@ +#include "https-utils.h"
+const char *url = "retrace.fedoraproject.org"; +bool ssl_allow_insecure = false; +bool http_show_headers = false; +unsigned port = 443;
+void alert_server_error() +{
- alert(_("An error occurred on the server side. Try again later."));
+}
+void alert_connection_error() +{
- alert(_("An error occurred while connecting to the server. "
"Check your network connection and try again."));
+}
+static SECStatus ssl_bad_cert_handler(void *arg, PRFileDesc *sock) +{
- PRErrorCode err = PR_GetError();
- CERTCertificate *cert = SSL_PeerCertificate(sock);
- char *subject = CERT_NameToAscii(&cert->subject);
- char *subject_cn = CERT_GetCommonName(&cert->subject);
- char *issuer = CERT_NameToAscii(&cert->issuer);
- CERT_DestroyCertificate(cert);
- char *target_host = SSL_RevealURL(sock);
- if (!target_host)
target_host = xstrdup("(unknown)");
- switch (err)
- {
- case SEC_ERROR_CA_CERT_INVALID:
error_msg(_("Issuer certificate is invalid: '%s'."), issuer);
break;
- case SEC_ERROR_UNTRUSTED_ISSUER:
error_msg(_("Certificate is signed by an untrusted issuer: '%s'."), issuer);
break;
- case SSL_ERROR_BAD_CERT_DOMAIN:
error_msg(_("Certificate subject name '%s' does not match target host name '%s'."),
subject_cn, target_host);
break;
- case SEC_ERROR_EXPIRED_CERTIFICATE:
error_msg(_("Remote certificate has expired."));
break;
- case SEC_ERROR_UNKNOWN_ISSUER:
error_msg(_("Certificate issuer is not recognized: '%s'."), issuer);
break;
- default:
error_msg(_("Bad certificate received. Subject '%s', issuer '%s'."),
subject, issuer);
break;
- }
- PR_Free(target_host);
- return ssl_allow_insecure ? SECSuccess : SECFailure;
+}
+static SECStatus ssl_handshake_callback(PRFileDesc *sock, void *arg) +{
- return SECSuccess;
+}
+static const char *ssl_get_configdir() +{
- struct stat buf;
- if (getenv("SSL_DIR"))
- {
if (0 == stat(getenv("SSL_DIR"), &buf) &&
S_ISDIR(buf.st_mode))
{
return getenv("SSL_DIR");
}
- }
- if (0 == stat("/etc/pki/nssdb", &buf) &&
S_ISDIR(buf.st_mode))
- {
return "/etc/pki/nssdb";
- }
- return NULL;
+}
+static PK11GenericObject *nss_load_cacert(const char *filename) +{
- PK11SlotInfo *slot = PK11_FindSlotByName("PEM Token #0");
- if (!slot)
error_msg_and_die(_("Failed to get slot 'PEM Token #0': %d."), PORT_GetError());
- CK_ATTRIBUTE template[4];
- CK_OBJECT_CLASS class = CKO_CERTIFICATE;
+#define PK11_SETATTRS(x,id,v,l) (x)->type = (id); \
- (x)->pValue=(v); (x)->ulValueLen = (l)
- PK11_SETATTRS(&template[0], CKA_CLASS, &class, sizeof(class));
- CK_BBOOL cktrue = CK_TRUE;
- PK11_SETATTRS(&template[1], CKA_TOKEN, &cktrue, sizeof(CK_BBOOL));
- PK11_SETATTRS(&template[2], CKA_LABEL, (unsigned char*)filename, strlen(filename)+1);
- PK11_SETATTRS(&template[3], CKA_TRUST, &cktrue, sizeof(CK_BBOOL));
- PK11GenericObject *cert = PK11_CreateGenericObject(slot, template, 4, PR_FALSE);
- PK11_FreeSlot(slot);
- return cert;
+}
+static char *ssl_get_password(PK11SlotInfo *slot, PRBool retry, void *arg) +{
- return NULL;
+}
+void ssl_connect(PRFileDesc **tcp_sock, PRFileDesc **ssl_sock) +{
- PRAddrInfo *addrinfo = PR_GetAddrInfoByName(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());
- }
- void *enumptr = NULL;
- PRNetAddr addr;
- *tcp_sock = NULL;
- while ((enumptr = PR_EnumerateAddrInfo(enumptr, addrinfo, port, &addr)))
- {
if (addr.raw.family == PR_AF_INET || addr.raw.family == PR_AF_INET6)
{
*tcp_sock = PR_OpenTCPSocket(addr.raw.family);
break;
}
- }
- PR_FreeAddrInfo(addrinfo);
- if (!*tcp_sock)
error_msg_and_die(_("Failed to create a TCP socket"));
- PRSocketOptionData sock_option;
- 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);
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);
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, url);
- if (SECSuccess != sec_status)
- {
PR_Close(*ssl_sock);
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)
- {
PR_Close(*ssl_sock);
alert_connection_error();
error_msg_and_die(_("Failed to connect SSL address."));
- }
- 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)
- {
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)
- {
PR_Close(*ssl_sock);
alert_server_error();
error_msg_and_die(_("Failed to force handshake: NSS error %d."),
PR_GetError());
- }
+}
+void ssl_disconnect(PRFileDesc *ssl_sock) +{
- PRStatus pr_status = PR_Close(ssl_sock);
- if (PR_SUCCESS != pr_status)
error_msg(_("Failed to close SSL socket."));
+}
+/**
- Parse a header's value from HTTP message. Only alnum values are supported.
- @returns
- Caller must free the returned value.
- If no header is found, NULL is returned.
- */
+char *http_get_header_value(const char *message,
const char *header_name)
+{
- char *headers_end = strstr(message, "\r\n\r\n");
- if (!headers_end)
return NULL;
- char *search_string = xasprintf("\r\n%s:", header_name);
- char *header = strcasestr(message, search_string);
- if (!header || header > headers_end)
return NULL;
- header += strlen(search_string);
- free(search_string);
- while (*header == ' ')
++header;
- int len = 0;
- while (header[len] && header[len] != '\r' && header[len] != '\n')
++len;
- while (header[len - 1] == ' ') /* strip spaces from right */
--len;
- return xstrndup(header, len);
+}
+/**
- Parse body from HTTP message.
- Caller must free the returned value.
- */
+char *http_get_body(const char *message) +{
- char *body = strstr(message, "\r\n\r\n");
- if (!body)
return NULL;
- body += strlen("\r\n\r\n");
- while (*body == ' ')
++body;
- int len = strlen(body);
- while (body[len - 1] == ' ')
--len;
- return xstrndup(body, len);
+}
+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, " ");
- if (!space)
- {
alert_server_error();
error_msg_and_die(_("Invalid response from server: HTTP header not found."));
- }
- 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."));
- }
- return response_code;
+}
+void http_print_headers(FILE *file, const char *message) +{
- const char *headers_end = strstr(message, "\r\n\r\n");
- const char *c;
- if (!headers_end)
headers_end = message + strlen(message);
- for (c = message; c != headers_end + 2; ++c)
- {
if (*c == '\r')
continue;
putc(*c, file);
- }
+}
+/**
- @returns
- Caller must free the returned value.
- */
+char *tcp_read_response(PRFileDesc *tcp_sock) +{
- struct strbuf *strbuf = strbuf_new();
- char buf[32768];
- PRInt32 received = 0;
- do {
received = PR_Recv(tcp_sock, buf, sizeof(buf) - 1, /*flags:*/0,
PR_INTERVAL_NO_TIMEOUT);
if (received > 0)
{
buf[received] = '\0';
strbuf_append_str(strbuf, buf);
}
if (received == -1)
{
alert_connection_error();
error_msg_and_die(_("Receiving of data failed: NSS error %d."),
PR_GetError());
}
- } while (received > 0);
- return strbuf_free_nobuf(strbuf);
+}
+void nss_init(SECMODModule **mod, PK11GenericObject **cert) +{
- SECStatus sec_status;
- const char *configdir = ssl_get_configdir();
- if (configdir)
sec_status = NSS_Initialize(configdir, "", "", "", NSS_INIT_READONLY);
- else
sec_status = NSS_NoDB_Init(NULL);
- if (SECSuccess != sec_status)
error_msg_and_die(_("Failed to initialize NSS."));
- char *user_module = xstrdup("library=libnsspem.so name=PEM");
- *mod = SECMOD_LoadUserModule(user_module, NULL, PR_FALSE);
- free(user_module);
- if (!*mod || !(*mod)->loaded)
error_msg_and_die(_("Failed to initialize security module."));
- *cert = nss_load_cacert("/etc/pki/tls/certs/ca-bundle.crt");
- PK11_SetPasswordFunc(ssl_get_password);
- NSS_SetDomesticPolicy();
+}
+void nss_close(SECMODModule *mod, PK11GenericObject *cert) +{
- SSL_ClearSessionCache();
- PK11_DestroyGenericObject(cert);
- SECMOD_UnloadUserModule(mod);
- SECMOD_DestroyModule(mod);
- SECStatus sec_status = NSS_Shutdown();
- if (SECSuccess != sec_status)
error_msg(_("Failed to shutdown NSS."));
- PR_Cleanup();
+} diff --git a/src/plugins/https-utils.h b/src/plugins/https-utils.h index e69de29..352de40 100644 --- a/src/plugins/https-utils.h +++ b/src/plugins/https-utils.h @@ -0,0 +1,27 @@
license missing and this is header so put around
#ifndef HTTS_UTILS_H #define HTTS_UTILS_H ... #endif
+#include <libreport/client.h> +#include <nspr.h> +#include <nss.h> +#include <pk11pub.h> +#include <ssl.h> +#include <sslproto.h> +#include <sslerr.h> +#include <secerr.h> +#include <secmod.h> +#include "libabrt.h"
+extern const char *url;
I don't like the way how it is shared between two different apps.
#define url_var(value) \ static const char *url = value;
+extern bool ssl_allow_insecure; +extern bool http_show_headers; +extern unsigned port;
+extern void alert_server_error(); +extern void alert_connection_error(); +extern void ssl_connect(PRFileDesc **tcp_sock, PRFileDesc **ssl_sock); +extern void ssl_disconnect(PRFileDesc *ssl_sock); +extern char *http_get_header_value(const char *message, const char *header_name); +extern char *http_get_body(const char *message); +extern int http_get_response_code(const char *message); +extern void http_print_headers(FILE *file, const char *message); +extern char *tcp_read_response(PRFileDesc *tcp_sock); +extern void nss_init(SECMODModule **mod, PK11GenericObject **cert); +extern void nss_close(SECMODModule *mod, PK11GenericObject *cert);
diff --git a/abrt.spec.in b/abrt.spec.in index 1c08ca0..b50c13d 100644 --- a/abrt.spec.in +++ b/abrt.spec.in @@ -470,6 +470,7 @@ gtk-update-icon-cache %{_datadir}/icons/hicolor &>/dev/null || : %{_bindir}/abrt-action-generate-core-backtrace %{_bindir}/abrt-action-analyze-backtrace %{_bindir}/abrt-action-list-dsos +%{_bindir}/abrt-dedup-client %{_sbindir}/abrt-install-ccpp-hook %{_sysconfdir}/libreport/events.d/ccpp_event.conf %{_sysconfdir}/libreport/events.d/gconf_event.conf diff --git a/po/POTFILES.in b/po/POTFILES.in index d2a37de..ec72e9d 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -19,6 +19,7 @@ src/plugins/abrt-action-generate-backtrace.c src/plugins/abrt-action-generate-core-backtrace.c src/plugins/abrt-action-install-debuginfo src/plugins/abrt-action-trim-files.c +src/plugins/abrt-dedup-client.c src/plugins/abrt-dump-oops.c src/plugins/abrt-retrace-client.c src/plugins/analyze_LocalGDB.xml.in diff --git a/src/plugins/Makefile.am b/src/plugins/Makefile.am index 2dc404c..0c850f7 100644 --- a/src/plugins/Makefile.am +++ b/src/plugins/Makefile.am @@ -15,7 +15,8 @@ bin_PROGRAMS = \ abrt-action-generate-backtrace \ abrt-action-generate-core-backtrace \ abrt-action-analyze-backtrace \
- abrt-retrace-client
- abrt-retrace-client \
- abrt-dedup-client
libexec_PROGRAMS = abrt-action-install-debuginfo-to-abrt-cache
@@ -212,4 +213,20 @@ abrt_retrace_client_SOURCES = \ $(BTPARSER_LIBS) \ $(NSS_LIBS)
+abrt_dedup_client_SOURCES = \
- abrt-dedup-client.c \
- https-utils.c
- abrt_dedup_client_CFLAGS = \
- -I$(srcdir)/../include \
- -I$(srcdir)/../lib \
$(NSS_CFLAGS) \
$(GLIB_CFLAGS) \
-D_GNU_SOURCE \
$(LIBREPORT_CFLAGS) \
-Wall -Wwrite-strings -Werror
- abrt_dedup_client_LDADD = \
$(LIBREPORT_LIBS) \
$(BTPARSER_LIBS) \
$(NSS_LIBS)
DEFS = -DLOCALEDIR="$(localedir)" @DEFS@ diff --git a/src/plugins/abrt-dedup-client.c b/src/plugins/abrt-dedup-client.c index e69de29..cfd424f 100644 --- a/src/plugins/abrt-dedup-client.c +++ b/src/plugins/abrt-dedup-client.c @@ -0,0 +1,267 @@ +/*
- Copyright (C) 2010 Red Hat, Inc.
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 2 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License along
- with this program; if not, write to the Free Software Foundation, Inc.,
- 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+*/ +#include <syslog.h> +#if HAVE_LOCALE_H +# include <locale.h> +#endif +#include "https-utils.h"
+struct dedup_bug_data +{
- int bug_id;
- char *component;
- int similarity;
- char *status;
- char *resolution;
+};
+static const char *component = NULL; +static const char *btfile = NULL;
+/* idea from curl */ +/* escape everything except [a-zA-Z0-9.-_~] */ +static inline int noescape(unsigned char c) +{
- if ((c >= 'a' && c <= 'z') ||
(c >= 'A' && c <= 'Z') ||
(c >= '0' && c <= '9') ||
c == '-' || c == '.' ||
c == '~' || c == '_')
return 1;
- return 0;
+}
+int main(int argc, char **argv) +{
- int result = 0;
- setlocale(LC_ALL, "");
+#if ENABLE_NLS
- bindtextdomain(PACKAGE, LOCALEDIR);
- textdomain(PACKAGE);
+#endif
- enum {
OPT_verbose = 1 << 0,
OPT_syslog = 1 << 1,
OPT_insecure = 1 << 2,
OPT_url = 1 << 3,
OPT_port = 1 << 4,
OPT_headers = 1 << 5,
- };
- /* Keep enum above and order of options below in sync! */
- struct options options[] = {
OPT__VERBOSE(&g_verbose),
OPT_BOOL('s', "syslog", NULL, _("log to syslog")),
OPT_BOOL('k', "insecure", NULL,
_("allow insecure connection to dedup server")),
OPT_STRING(0, "url", &url, "URL",
_("dedup server URL")),
OPT_INTEGER(0, "port", &port,
_("dedup server port")),
OPT_BOOL(0, "headers", NULL,
_("(debug) show received HTTP headers")),
OPT_END()
- };
- const char *usage = _("abrt-dedup-client component backtrace_file [options]");
- char *env_url = getenv("DEDUP_SERVER_URL");
- if (env_url)
url = env_url;
- char *env_port = getenv("DEDUP_SERVER_PORT");
- if (env_port)
port = xatou(env_port);
- char *env_insecure = getenv("DEDUP_SERVER_INSECURE");
- if (env_insecure)
ssl_allow_insecure = strncmp(env_insecure, "insecure", strlen("insecure")) == 0;
- unsigned opts = parse_opts(argc, argv, options, usage);
- if (opts & OPT_syslog)
- {
openlog(msg_prefix, 0, LOG_DAEMON);
logmode = LOGMODE_SYSLOG;
- }
- if (argc - optind != 2)
show_usage_and_die(usage, options);
clearest way
argv += optind; if (!argv[0]) show_usage_and_die(program_usage_string, program_options);
- if (!ssl_allow_insecure)
ssl_allow_insecure = opts & OPT_insecure;
- http_show_headers = opts & OPT_headers;
- component = argv[optind];
- btfile = argv[optind + 1];
- int i;
- /* just a simple check (eg. allows '~' which should not be present in component name) */
- for (i = 0; component[i]; ++i)
if (!noescape(component[i]))
error_msg_and_die(_("Forbidden character in component name: '%c'."), component[i]);
why not?
- FILE *fp = fopen(btfile, "r");
- if (!fp)
error_msg_and_die(_("Unable to open file '%s'."), btfile);
- struct strbuf *request_body = strbuf_new();
- strbuf_append_strf(request_body, "component=%s&backtrace=", component);
component memtest89+ wont be working, because '+' is not escaped char *escaped = g_uri_escape_string(component, NULL, 0);
- char buf[1024];
- unsigned char c;
- /* read buffered and escape at the same time */
- /* => just use one strbuf instead of two */
- while (fgets(buf, sizeof(buf), fp))
- {
for (i = 0; buf[i]; ++i)
{
c = (unsigned char)buf[i];
if (noescape(c))
strbuf_append_char(request_body, c);
else
strbuf_append_strf(request_body, "%%%02X", c);
}
- }
also here you can use glib
- fclose(fp);
- struct strbuf *request = strbuf_new();
- strbuf_append_strf(request,
"POST /btserver.cgi HTTP/1.1\r\n"
"Host: %s\r\n"
"Content-Type: application/x-www-form-urlencoded\r\n"
"Content-Length: %d\r\n"
"Connection: close\r\n"
"\r\n"
"%s",
url, request_body->len, request_body->buf);
- strbuf_free(request_body);
- /* Initialize NSS */
- SECMODModule *mod;
- PK11GenericObject *cert;
- nss_init(&mod, &cert);
- /* start SSL communication */
- PRFileDesc *tcp_sock, *ssl_sock;
- ssl_connect(&tcp_sock, &ssl_sock);
- PRInt32 written = PR_Send(tcp_sock, request->buf, request->len,
0/*flags*/, PR_INTERVAL_NO_TIMEOUT);
- if (written < 0)
- {
PR_Close(ssl_sock);
alert_connection_error();
error_msg_and_die(_("Failed to send HTTP request of length %d: NSS error %d."),
request->len, PR_GetError());
- }
- char *response = tcp_read_response(tcp_sock);
- strbuf_free(request);
- ssl_disconnect(ssl_sock);
- if (http_show_headers)
http_print_headers(stdout, response);
- char *response_body = http_get_body(response);
- char *encoding = http_get_header_value(response, "Transfer-Encoding");
- if (encoding && strcasecmp("chunked", encoding) == 0)
- {
char *newbody = http_join_chunked(response_body, strlen(response_body));
free(response_body);
response_body = newbody;
- }
- free(encoding);
- struct dedup_bug_data bug;
- /* iterate line by line */
- char *line = response_body, *newline = response_body;
- int last = 0;
- while (!last)
- {
/* split lines */
while (*newline && *newline != '\n' && *newline != '\r')
++newline;
/* end of input => last line */
if (!*newline)
last = 1;
*newline = '\0';
/* we could have stopped at \r, ++newline would be \n */
if (!last)
while (newline[1] && (newline[1] == '\n' || newline[1] == '\r'))
++newline;
if (sscanf(line, "%d", &bug.bug_id) != 1)
{
line = ++newline;
continue;
}
bug.component = line;
while (*bug.component && (isspace(*bug.component) || isdigit(*bug.component)))
++bug.component;
bug.status = bug.component;
while (*bug.status && !isspace(*bug.status))
++bug.status;
*bug.status = '\0';
++bug.status;
if (sscanf(bug.status, "%d", &bug.similarity) != 1)
{
line = ++newline;
continue;
}
while (*bug.status && (isspace(*bug.status) || isdigit(*bug.status)))
++bug.status;
bug.resolution = NULL;
if (strncmp(bug.status, "CLOSED", strlen("CLOSED")) == 0)
{
bug.resolution = bug.status;
while (*bug.resolution && !isspace(*bug.resolution))
++bug.resolution;
*bug.resolution = '\0';
++bug.resolution;
while (*bug.resolution && isspace(*bug.resolution))
++bug.resolution;
}
printf(_("We have detected that your problem is a possible duplicate "
"of bug #%d (from %s).\n"), bug.bug_id, bug.component);
if (bug.resolution)
printf(_("It has already been closed as %s. You can check "
"Bugzilla for a solution.\n"), bug.resolution);
else
printf(_("The bug has not yet been fixed. Feel free to continue "
"reporting and provide as many information as you can.\n"));
line = ++newline;
- }
live is short to write own token parser, use the one from glibc
char *str1, *str2, *token, *subtoken; char *saveptr1, *saveptr2;
for (str1 = t; ; str1 = NULL) { token = strtok_r(str1, "\r\n", &saveptr1); if (token == NULL) break; printf("%s\n", token);
for (str2 = token; ; str2 = NULL) { subtoken = strtok_r(str2, " ", &saveptr2); if (subtoken == NULL) break; printf(" --> %s\n", subtoken); } }
- free(response);
- free(response_body);
- /* Shutdown NSS. */
- nss_close(mod, cert);
- return result;
+} diff --git a/src/plugins/https-utils.c b/src/plugins/https-utils.c index e5ed7b5..84177bf 100644 --- a/src/plugins/https-utils.c +++ b/src/plugins/https-utils.c @@ -324,6 +324,43 @@ char *tcp_read_response(PRFileDesc *tcp_sock) return strbuf_free_nobuf(strbuf); }
+/**
- Joins HTTP response body if the Transfer-Encoding is chunked.
- @param body raw HTTP response body (response without headers)
the function operates on the input, but returns it
to the initial state when done
- @returns Joined HTTP response body. Caller must free the value.
+*/ +char *http_join_chunked(char *body, int bodylen) +{
- struct strbuf *result = strbuf_new();
- unsigned len;
- int blen = bodylen > 0 ? bodylen : strlen(body);
- char prevchar;
- char *cursor = body;
- while (cursor - body < blen)
- {
if (sscanf(cursor, "%x", &len) != 1)
break;
/* jump to next line */
while (*cursor && *cursor != '\n')
++cursor;
++cursor;
simple way
while (t && *t && *t++ != '\n') ;
/* split chunk and append to result */
prevchar = cursor[len];
cursor[len] = '\0';
strbuf_append_str(result, cursor);
cursor[len] = prevchar;
/* len + strlen("\r\n") */
cursor += len + 2;
- }
- return strbuf_free_nobuf(result);
+}
void nss_init(SECMODModule **mod, PK11GenericObject **cert) { SECStatus sec_status; diff --git a/src/plugins/https-utils.h b/src/plugins/https-utils.h index 352de40..1961716 100644 --- a/src/plugins/https-utils.h +++ b/src/plugins/https-utils.h @@ -23,5 +23,6 @@ extern char *http_get_body(const char *message); extern int http_get_response_code(const char *message); extern void http_print_headers(FILE *file, const char *message); extern char *tcp_read_response(PRFileDesc *tcp_sock); +extern char *http_join_chunked(char *body, int bodylen); extern void nss_init(SECMODModule **mod, PK11GenericObject **cert); extern void nss_close(SECMODModule *mod, PK11GenericObject *cert);
On 27.01.2012 16:04, Nikola Pajkovsky wrote:
-#define PK11_SETATTRS(x,id,v,l) (x)->type = (id); \
- (x)->pValue=(v); (x)->ulValueLen = (l)
please consider to use 'do { ... } while(0)' in macro
#define PK11_SETATTRS(x,id,v,l) \ do { \ (x)->type = (id); \ (x)->pValue = (v); \ (x)->ulValueLen = (l) \ } while(0)
Done.
diff --git a/src/plugins/https-utils.h b/src/plugins/https-utils.h index e69de29..352de40 100644 --- a/src/plugins/https-utils.h +++ b/src/plugins/https-utils.h @@ -0,0 +1,27 @@
license missing and this is header so put around
#ifndef HTTS_UTILS_H #define HTTS_UTILS_H ... #endif
Done.
+#include "libabrt.h"
+extern const char *url;
I don't like the way how it is shared between two different apps.
#define url_var(value) \ static const char *url = value;
Wouldn't work. url is used as a global in both https-utils.c & abrt-{retrace,dedup}-client.c
- if (argc - optind != 2)
show_usage_and_die(usage, options);
clearest way
argv += optind; if (!argv[0]) show_usage_and_die(program_usage_string, program_options);
Done.
- int i;
- /* just a simple check (eg. allows '~' which should not be present in component name) */
- for (i = 0; component[i]; ++i)
if (!noescape(component[i]))
error_msg_and_die(_("Forbidden character in component name: '%c'."), component[i]);
why not?
Naming Guidelines: allowed characters [0-9a-zA-Z_-.+] (ohh, plus :( ) Fixed.
- FILE *fp = fopen(btfile, "r");
- if (!fp)
error_msg_and_die(_("Unable to open file '%s'."), btfile);
- struct strbuf *request_body = strbuf_new();
- strbuf_append_strf(request_body, "component=%s&backtrace=", component);
component memtest89+ wont be working, because '+' is not escaped char *escaped = g_uri_escape_string(component, NULL, 0);
Good point, plus needs to be allowed & escaped. However I don't like the idea of linking with glib because of one function (especially a simple one).
- char buf[1024];
- unsigned char c;
- /* read buffered and escape at the same time */
- /* => just use one strbuf instead of two */
- while (fgets(buf, sizeof(buf), fp))
- {
for (i = 0; buf[i]; ++i)
{
c = (unsigned char)buf[i];
if (noescape(c))
strbuf_append_char(request_body, c);
else
strbuf_append_strf(request_body, "%%%02X", c);
}
- }
also here you can use glib
Same.
- struct dedup_bug_data bug;
- /* iterate line by line */
- char *line = response_body, *newline = response_body;
- int last = 0;
- while (!last)
- {
/* split lines */
while (*newline&& *newline != '\n'&& *newline != '\r')
++newline;
/* end of input => last line */
if (!*newline)
last = 1;
*newline = '\0';
/* we could have stopped at \r, ++newline would be \n */
if (!last)
while (newline[1]&& (newline[1] == '\n' || newline[1] == '\r'))
++newline;
if (sscanf(line, "%d",&bug.bug_id) != 1)
{
line = ++newline;
continue;
}
bug.component = line;
while (*bug.component&& (isspace(*bug.component) || isdigit(*bug.component)))
++bug.component;
bug.status = bug.component;
while (*bug.status&& !isspace(*bug.status))
++bug.status;
*bug.status = '\0';
++bug.status;
if (sscanf(bug.status, "%d",&bug.similarity) != 1)
{
line = ++newline;
continue;
}
while (*bug.status&& (isspace(*bug.status) || isdigit(*bug.status)))
++bug.status;
bug.resolution = NULL;
if (strncmp(bug.status, "CLOSED", strlen("CLOSED")) == 0)
{
bug.resolution = bug.status;
while (*bug.resolution&& !isspace(*bug.resolution))
++bug.resolution;
*bug.resolution = '\0';
++bug.resolution;
while (*bug.resolution&& isspace(*bug.resolution))
++bug.resolution;
}
printf(_("We have detected that your problem is a possible duplicate "
"of bug #%d (from %s).\n"), bug.bug_id, bug.component);
if (bug.resolution)
printf(_("It has already been closed as %s. You can check "
"Bugzilla for a solution.\n"), bug.resolution);
else
printf(_("The bug has not yet been fixed. Feel free to continue "
"reporting and provide as many information as you can.\n"));
line = ++newline;
- }
live is short to write own token parser, use the one from glibc
char *str1, *str2, *token, *subtoken; char *saveptr1, *saveptr2;
for (str1 = t; ; str1 = NULL) { token = strtok_r(str1, "\r\n",&saveptr1); if (token == NULL) break; printf("%s\n", token);
for (str2 = token; ; str2 = NULL) { subtoken = strtok_r(str2, " ",&saveptr2); if (subtoken == NULL) break; printf(" --> %s\n", subtoken); }
}
I agree, it will be more simple using str* functions => needs to be rewritten completely.
/* jump to next line */
while (*cursor&& *cursor != '\n')
++cursor;
++cursor;
simple way
while (t&& *t&& *t++ != '\n') ;
Seems much less readable to me. Anyway, strchr would probably be better: cursor = strchr(cursor, '\n'); if (cursor) ++cursor;
Rewritten based on previous discussions. The dedup-client now does not really understand the response, it just splits the human-readable and machine-parseable messages. Please review.
Michal
Michal Toman mtoman@redhat.com writes:
From dc9e513e75ea67b0b84ffa45a8b58e00b79a2774 Mon Sep 17 00:00:00 2001 From: Michal Toman mtoman@redhat.com Date: Mon, 6 Feb 2012 15:31:01 +0100 Subject: [PATCH 2/2] add abrt-dedup-client
abrt.spec.in | 1 + po/POTFILES.in | 1 + src/plugins/Makefile.am | 19 +++- src/plugins/abrt-dedup-client.c | 278 +++++++++++++++++++++++++++++++++++++++ src/plugins/https-utils.c | 38 ++++++ src/plugins/https-utils.h | 1 + 6 files changed, 337 insertions(+), 1 deletions(-) create mode 100644 src/plugins/abrt-dedup-client.c
+%{_bindir}/abrt-dedup-client
it would be nice to put dedup bin into .gitignore
diff --git a/src/plugins/abrt-dedup-client.c b/src/plugins/abrt-dedup-client.c new file mode 100644 index 0000000..cecc4d8 --- /dev/null +++ b/src/plugins/abrt-dedup-client.c +static inline int is_valid_component_name(const char *name) +{
- int i;
- unsigned char c;
- for (i = 0; name[i]; ++i)
- {
c = (unsigned char)name[i];
if ((c < 'a' || c > 'z') &&
(c < 'A' || c > 'Z') &&
(c < '0' || c > '9') &&
it's isalnum ^^
+static void escape(const char *src, struct strbuf *dest) +{
- int i;
- unsigned char c;
- for (i = 0; src[i]; ++i)
- {
c = (unsigned char)src[i];
if ((c >= 'a' && c <= 'z') ||
(c >= 'A' && c <= 'Z') ||
(c >= '0' && c <= '9') ||
ditto
OPT_BOOL('k', "insecure", NULL,
_("allow insecure connection to dedup server")),
- if (!ssl_allow_insecure)
ssl_allow_insecure = opts & OPT_insecure;
ssl_allow_insecure is hardcoded to false, if is always true
- if (!dump_dir_name)
dump_dir_name = strdup(".");
dump_dir_name is never NULL, because it's static variable defined at the beginning of source code
- struct strbuf *request_body = strbuf_new();
- strbuf_append_str(request_body, "component=");
- /* escape component - may contain '+' */
- escape(component, request_body);
- free(component);
- strbuf_append_str(request_body, "&backtrace=");
- escape(backtrace, request_body);
when I'm thinking about the escape function it will be better to introduce two new *strbuf_append_escape_str[f] functions.
- /* iterate line by line */
- char *line = response_body, *newline = response_body;
- for (line = response_body; line; line = newline)
- {
twice association response_body to line variable
for ( ; line; line = newline)
or
char *newline = response_body; for (char *line; line; line = newline)
On 08.02.2012 15:04, Nikola Pajkovsky wrote:
+%{_bindir}/abrt-dedup-client
it would be nice to put dedup bin into .gitignore
done
diff --git a/src/plugins/abrt-dedup-client.c b/src/plugins/abrt-dedup-client.c new file mode 100644 index 0000000..cecc4d8 --- /dev/null +++ b/src/plugins/abrt-dedup-client.c +static inline int is_valid_component_name(const char *name) +{
- int i;
- unsigned char c;
- for (i = 0; name[i]; ++i)
- {
c = (unsigned char)name[i];
if ((c< 'a' || c> 'z')&&
(c< 'A' || c> 'Z')&&
(c< '0' || c> '9')&&
it's isalnum ^^
done
+static void escape(const char *src, struct strbuf *dest) +{
- int i;
- unsigned char c;
- for (i = 0; src[i]; ++i)
- {
c = (unsigned char)src[i];
if ((c>= 'a'&& c<= 'z') ||
(c>= 'A'&& c<= 'Z') ||
(c>= '0'&& c<= '9') ||
done
OPT_BOOL('k', "insecure", NULL,
_("allow insecure connection to dedup server")),
- if (!ssl_allow_insecure)
ssl_allow_insecure = opts& OPT_insecure;
ssl_allow_insecure is hardcoded to false, if is always true
it is read from environment few lines above
- if (!dump_dir_name)
dump_dir_name = strdup(".");
dump_dir_name is never NULL, because it's static variable defined at the beginning of source code
removed
- struct strbuf *request_body = strbuf_new();
- strbuf_append_str(request_body, "component=");
- /* escape component - may contain '+' */
- escape(component, request_body);
- free(component);
- strbuf_append_str(request_body, "&backtrace=");
- escape(backtrace, request_body);
when I'm thinking about the escape function it will be better to introduce two new *strbuf_append_escape_str[f] functions.
can be split whenever another program needs it
- /* iterate line by line */
- char *line = response_body, *newline = response_body;
- for (line = response_body; line; line = newline)
- {
twice association response_body to line variable
for ( ; line; line = newline)
or
char *newline = response_body; for (char *line; line; line = newline)
fixed
Michal Toman mtoman@redhat.com writes:
From 3f18b10b8b416839496e50d6b8c83f08c2a9193e Mon Sep 17 00:00:00 2001 From: Michal Toman mtoman@redhat.com Date: Fri, 3 Feb 2012 10:19:43 +0100 Subject: [PATCH 1/2] split HTTPS util functions to a separate file
po/POTFILES.in | 1 + src/plugins/Makefile.am | 6 +- src/plugins/abrt-retrace-client.c | 394 +----------------------------------- src/plugins/https-utils.c | 403 +++++++++++++++++++++++++++++++++++++ src/plugins/https-utils.h | 62 ++++++ 5 files changed, 475 insertions(+), 391 deletions(-) create mode 100644 src/plugins/https-utils.c create mode 100644 src/plugins/https-utils.h +char *http_get_header_value(const char *message,
const char *header_name)
+{
- char *headers_end = strstr(message, "\r\n\r\n");
- if (!headers_end)
return NULL;
- char *search_string = xasprintf("\r\n%s:", header_name);
- char *header = strcasestr(message, search_string);
- if (!header || header > headers_end)
return NULL;
you are leaking search_string
+char *http_get_body(const char *message) +{
- char *body = strstr(message, "\r\n\r\n");
- if (!body)
return NULL;
- body += strlen("\r\n\r\n");
- while (*body == ' ')
++body;
- int len = strlen(body);
- while (body[len - 1] == ' ')
--len;
use strtrimch, it will trim from left and right specified char
diff --git a/src/plugins/https-utils.h b/src/plugins/https-utils.h new file mode 100644 index 0000000..a6b53a8 --- /dev/null +++ b/src/plugins/https-utils.h @@ -0,0 +1,62 @@ +/*
- Copyright (C) 2012 ABRT Team
- Copyright (C) 2012 Red Hat, Inc.
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 2 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License along
- with this program; if not, write to the Free Software Foundation, Inc.,
- 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+*/
+#ifndef ABRT_HTTPS_UTILS_H_ +#define ABRT_HTTPS_UTILS_H_
+#include <libreport/client.h> +#include <nspr.h> +#include <nss.h> +#include <pk11pub.h> +#include <ssl.h> +#include <sslproto.h> +#include <sslerr.h> +#include <secerr.h> +#include <secmod.h> +#include "libabrt.h"
+#if HAVE_LOCALE_H +#include <locale.h> +#endif
+struct language +{
- char *locale;
- char *encoding;
+};
+extern const char *url; +extern bool ssl_allow_insecure; +extern bool http_show_headers; +extern unsigned port;
according to me, this is way better solution, than extern 4 variables from one binary to another through header
struct https_utls { const char *url; bool ssl_allow_insecure; bool http_show_headers; unsigned port; };
#define INIT_HTTPS_UTLS(name) \ static struct https_utls name = { \ .url = "some-url", \ .ssl_allow_insecure = true, \ .http_show_headers = false, \ .port = 100, \ }
INIT_HTTPS_UTLS(https_struct);
+extern void get_language(struct language *lang); +extern void alert_server_error(); +extern void alert_connection_error(); +extern void ssl_connect(PRFileDesc **tcp_sock, PRFileDesc **ssl_sock); +extern void ssl_disconnect(PRFileDesc *ssl_sock); +extern char *http_get_header_value(const char *message, const char *header_name); +extern char *http_get_body(const char *message); +extern int http_get_response_code(const char *message); +extern void http_print_headers(FILE *file, const char *message); +extern char *tcp_read_response(PRFileDesc *tcp_sock); +extern void nss_init(SECMODModule **mod, PK11GenericObject **cert); +extern void nss_close(SECMODModule *mod, PK11GenericObject *cert);
you don't have to extern; if you have header foo.h, foo.c and function bar() which is in foo.h, but not belongs to foo.c it's good to prefix it with extern. otherwise it's pointless to use extern.
On 08.02.2012 14:47, Nikola Pajkovsky wrote:
+char *http_get_header_value(const char *message,
const char *header_name)
+{
- char *headers_end = strstr(message, "\r\n\r\n");
- if (!headers_end)
return NULL;
- char *search_string = xasprintf("\r\n%s:", header_name);
- char *header = strcasestr(message, search_string);
- if (!header || header> headers_end)
return NULL;
you are leaking search_string
fixed
+char *http_get_body(const char *message) +{
- char *body = strstr(message, "\r\n\r\n");
- if (!body)
return NULL;
- body += strlen("\r\n\r\n");
- while (*body == ' ')
++body;
- int len = strlen(body);
- while (body[len - 1] == ' ')
--len;
use strtrimch, it will trim from left and right specified char
done
+struct language +{
- char *locale;
- char *encoding;
+};
+extern const char *url; +extern bool ssl_allow_insecure; +extern bool http_show_headers; +extern unsigned port;
according to me, this is way better solution, than extern 4 variables from one binary to another through header
struct https_utls { const char *url; bool ssl_allow_insecure; bool http_show_headers; unsigned port; };
#define INIT_HTTPS_UTLS(name) \ static struct https_utls name = { \ .url = "some-url", \ .ssl_allow_insecure = true, \ .http_show_headers = false, \ .port = 100, \ }
INIT_HTTPS_UTLS(https_struct);
hopefully done :) ssl_allow_insecure needs to be a global, because it is used in a callback function with fixed parameters. However it does not need to be shared anymore, so it became static in https-utils.c
+extern void get_language(struct language *lang); +extern void alert_server_error(); +extern void alert_connection_error(); +extern void ssl_connect(PRFileDesc **tcp_sock, PRFileDesc **ssl_sock); +extern void ssl_disconnect(PRFileDesc *ssl_sock); +extern char *http_get_header_value(const char *message, const char *header_name); +extern char *http_get_body(const char *message); +extern int http_get_response_code(const char *message); +extern void http_print_headers(FILE *file, const char *message); +extern char *tcp_read_response(PRFileDesc *tcp_sock); +extern void nss_init(SECMODModule **mod, PK11GenericObject **cert); +extern void nss_close(SECMODModule *mod, PK11GenericObject *cert);
you don't have to extern; if you have header foo.h, foo.c and function bar() which is in foo.h, but not belongs to foo.c it's good to prefix it with extern. otherwise it's pointless to use extern.
done
crash-catcher@lists.fedorahosted.org