Created
March 30, 2016 16:35
-
-
Save psychon/73344f301e8b6947f0512374e978c771 to your computer and use it in GitHub Desktop.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
diff --git a/include/entry.h b/include/entry.h | |
index e604586..79f9d1f 100644 | |
--- a/include/entry.h | |
+++ b/include/entry.h | |
@@ -70,7 +70,7 @@ typedef struct xcb_xrm_component_t { | |
/** Used in xcb_xrm_entry_parse. */ | |
typedef struct xcb_xrm_entry_parser_state_t { | |
xcb_xrm_entry_parser_chunk_status_t chunk; | |
- char buffer[4096]; | |
+ char buffer[4096]; /* Let's overflow that! */ | |
char *buffer_pos; | |
xcb_xrm_binding_type_t current_binding_type; | |
} xcb_xrm_entry_parser_state_t; | |
diff --git a/include/match.h b/include/match.h | |
index 94984d0..cce4c83 100644 | |
--- a/include/match.h | |
+++ b/include/match.h | |
@@ -55,7 +55,7 @@ typedef struct xcb_xrm_match_t { | |
xcb_xrm_entry_t *entry; | |
/* An array where the n-th element describes how the n-th element of the | |
* query strings was matched. */ | |
- int *flags; | |
+ xcb_xrm_match_flags_t *flags; | |
} xcb_xrm_match_t; | |
/** | |
diff --git a/include/util.h b/include/util.h | |
index 4aea580..6f1818b 100644 | |
--- a/include/util.h | |
+++ b/include/util.h | |
@@ -39,10 +39,8 @@ | |
#define FREE(p) \ | |
do { \ | |
- if (p != NULL) { \ | |
- free(p); \ | |
- p = NULL; \ | |
- } \ | |
+ free(p); \ | |
+ p = NULL; \ | |
} while (0) | |
#undef MAX | |
diff --git a/include/xcb_xrm.h b/include/xcb_xrm.h | |
index 0a27b5a..b31624c 100644 | |
--- a/include/xcb_xrm.h | |
+++ b/include/xcb_xrm.h | |
@@ -29,7 +29,8 @@ | |
#ifndef __XCB_XRM_H__ | |
#define __XCB_XRM_H__ | |
-#include "externals.h" | |
+#include <stdbool.h> | |
+#include <xcb/xcb.h> | |
#ifdef __cplusplus | |
extern "C" { | |
@@ -55,6 +56,11 @@ extern "C" { | |
* xcb_screen_t *screen = xcb_aux_get_screen(conn, screennr); | |
* | |
* xcb_xrm_database_t *database = xcb_xrm_database_from_resource_manager(conn, screen); | |
+ * FIXME: There should be a function like XGetDefault(): | |
+ * - Try to get the DB from the server. | |
+ * - If nothing was found, check ~/.Xdefaults | |
+ * - For some reason this then gets the XENVIRONMENT env var (defaulting to | |
+ * ~/.Xdefaults-) and merges the two databases | |
* | |
* xcb_xrm_resource_t *resource; | |
* if (xcb_xrm_resource_get(database, "Xft.dpi", "Xft.dpi", &resource) < 0) { | |
@@ -98,8 +104,9 @@ typedef struct xcb_xrm_resource_t xcb_xrm_resource_t; | |
/** | |
* Loads the RESOURCE_MANAGER property and creates a database with its | |
- * contents. If the database could not be created, thie function will return | |
+ * contents. If the database could not be created, this function will return | |
* NULL. | |
+ * This function handles #include-directives (TODO: Should it?). | |
* | |
* @param conn A working XCB connection. | |
* @param screen The xcb_screen_t* screen to use. | |
@@ -112,6 +119,7 @@ xcb_xrm_database_t *xcb_xrm_database_from_resource_manager(xcb_connection_t *con | |
/** | |
* Creates a database from the given string. | |
* If the database could not be created, this function will return NULL. | |
+ * This function handles #include-directives. | |
* | |
* @param str The resource string. | |
* @returns The database described by the resource string. | |
@@ -123,6 +131,7 @@ xcb_xrm_database_t *xcb_xrm_database_from_string(const char *str); | |
/** | |
* Creates a database from a given file. | |
* If the file cannot be found or opened, NULL is returned. | |
+ * This function handles #include-directives. | |
* | |
* @param filename Valid filename. | |
* @returns The database described by the file's contents. | |
@@ -145,6 +154,8 @@ char *xcb_xrm_database_to_string(xcb_xrm_database_t *database); | |
* overridden if override is set; otherwise, the value is discarded. | |
* The source database will implicitly be free'd and must not be used | |
* afterwards. | |
+ * That's a pity. Wouldn't it be more useful to keep the source_db alive? That | |
+ * might make it easier to track the life time of databases. | |
* If NULL is passed for target_db, a new and empty database will be created | |
* and returned in the pointer. | |
* | |
@@ -202,8 +213,9 @@ void xcb_xrm_database_free(xcb_xrm_database_t *database); | |
* components as the resource name. | |
* @param resource A pointer to a xcb_xrm_resource_t* which will be modified to | |
* contain the matched resource. Note that this resource must be free'd by the | |
- * caller. | |
+ * caller via a call to @ref xcb_xrm_resource_free (). | |
* @return 0 on success, a negative error code otherwise. | |
+ * What's the meaning of "error code"? | |
*/ | |
int xcb_xrm_resource_get(xcb_xrm_database_t *database, const char *res_name, const char *res_class, | |
xcb_xrm_resource_t **resource); | |
@@ -211,6 +223,9 @@ int xcb_xrm_resource_get(xcb_xrm_database_t *database, const char *res_name, con | |
/** | |
* Returns the string value of the resource. | |
* The string is owned by the caller and must be free'd. | |
+ * Why the copy? Wouldn't it be easier to return a const char that is valid as | |
+ * long as the xcb_xrm_resource_t and callers who want a copy can strdup() | |
+ * themselves? | |
* | |
* @param resource The resource to use. | |
* @returns The string value of the given resource. | |
diff --git a/src/database.c b/src/database.c | |
index cba96cf..d5cec58 100644 | |
--- a/src/database.c | |
+++ b/src/database.c | |
@@ -37,7 +37,7 @@ void xcb_xrm_database_put(xcb_xrm_database_t *database, xcb_xrm_entry_t *entry, | |
/* | |
* Loads the RESOURCE_MANAGER property and creates a database with its | |
- * contents. If the database could not be created, thie function will return | |
+ * contents. If the database could not be created, this function will return | |
* NULL. | |
* | |
* @param conn A working XCB connection. | |
@@ -133,23 +133,20 @@ xcb_xrm_database_t *xcb_xrm_database_from_string(const char *_str) { | |
char *filename; | |
int j = strlen(line) - 1; | |
- /* Skip whitespace */ | |
+ /* Skip whitespace and any number of quotes */ | |
while (line[i] == ' ' || line[i] == '\t' || line[i] == '"') | |
i++; | |
while (line[j] == ' ' || line[j] == '\t' || line[j] == '"') | |
j--; | |
- filename = scalloc(1, j - i + 2); | |
- if (filename == NULL) | |
+ if (j < i) | |
+ /* Only whitespace left in the line */ | |
continue; | |
- memcpy(filename, &line[i], j - i + 1); | |
- filename[j - 1 + 1] = '\0'; | |
- | |
- // TODO XXX Filename globbing | |
+ filename = &line[i]; | |
+ line[j+1] = '\0'; | |
included = xcb_xrm_database_from_file(filename); | |
- FREE(filename); | |
if (included != NULL) | |
xcb_xrm_database_combine(included, &database, true); | |
@@ -199,7 +196,11 @@ char *xcb_xrm_database_to_string(xcb_xrm_database_t *database) { | |
TAILQ_FOREACH(entry, database, entries) { | |
char *entry_str = xcb_xrm_entry_to_string(entry); | |
char *tmp; | |
- sasprintf(&tmp, "%s%s\n", result == NULL ? "" : result, entry_str); | |
+ if (sasprintf(&tmp, "%s%s\n", result == NULL ? "" : result, entry_str) < 0) { | |
+ FREE(entry_str); | |
+ FREE(result); | |
+ return NULL; | |
+ } | |
FREE(entry_str); | |
FREE(result); | |
result = tmp; | |
@@ -264,7 +265,12 @@ void xcb_xrm_database_put_resource(xcb_xrm_database_t **database, const char *re | |
*database = xcb_xrm_database_from_string(""); | |
escaped = xcb_xrm_entry_escape_value(value); | |
- sasprintf(&line, "%s: %s", resource, escaped); | |
+ if (escaped == NULL) | |
+ return; | |
+ if (sasprintf(&line, "%s: %s", resource, escaped) < 0) { | |
+ FREE(escaped); | |
+ return; | |
+ } | |
FREE(escaped); | |
xcb_xrm_database_put_resource_line(database, line); | |
FREE(line); | |
@@ -295,7 +301,7 @@ void xcb_xrm_database_put_resource_line(xcb_xrm_database_t **database, const cha | |
if (xcb_xrm_entry_parse(line, &entry, false) == 0) { | |
xcb_xrm_database_put(*database, entry, true); | |
} else { | |
- xcb_xrm_entry_free(entry); | |
+ xcb_xrm_entry_free(entry); /* XXX: This is a double-free, I think */ | |
} | |
} | |
@@ -322,7 +328,7 @@ void xcb_xrm_database_free(xcb_xrm_database_t *database) { | |
void xcb_xrm_database_put(xcb_xrm_database_t *database, xcb_xrm_entry_t *entry, bool override) { | |
xcb_xrm_entry_t *current; | |
- if (entry == NULL) | |
+ if (database == NULL || entry == NULL) | |
return; | |
/* Let's see whether this is a duplicate entry. */ | |
diff --git a/src/entry.c b/src/entry.c | |
index 2e37063..6b86b05 100644 | |
--- a/src/entry.c | |
+++ b/src/entry.c | |
@@ -102,7 +102,7 @@ int xcb_xrm_entry_parse(const char *_str, xcb_xrm_entry_t **_entry, bool resourc | |
char *str; | |
xcb_xrm_entry_t *entry = NULL; | |
xcb_xrm_component_t *last; | |
- char value_buf[4096]; | |
+ char value_buf[4096]; /* Overflow me */ | |
char *value_pos = value_buf; | |
xcb_xrm_entry_parser_state_t state = { | |
@@ -164,6 +164,9 @@ int xcb_xrm_entry_parse(const char *_str, xcb_xrm_entry_t **_entry, bool resourc | |
goto process_normally; | |
case ':': | |
if (resource_only) { | |
+ /* This suggests that this should be split into two parsers, | |
+ * one resource_only, one for the value | |
+ */ | |
goto done_error; | |
} | |
@@ -323,11 +326,14 @@ char *xcb_xrm_entry_to_string(xcb_xrm_entry_t *entry) { | |
assert(entry != NULL); | |
TAILQ_FOREACH(component, &(entry->components), components) { | |
char *tmp; | |
- sasprintf(&tmp, "%s%s%s", result == NULL ? "" : result, | |
+ if (sasprintf(&tmp, "%s%s%s", result == NULL ? "" : result, | |
(is_first && component->binding_type == BT_TIGHT) | |
? "" | |
: (component->binding_type == BT_TIGHT ? "." : "*"), | |
- component->type == CT_NORMAL ? component->name : "?"); | |
+ component->type == CT_NORMAL ? component->name : "?") < 0) { | |
+ FREE(result); | |
+ return NULL; | |
+ } | |
FREE(result); | |
result = tmp; | |
@@ -335,7 +341,11 @@ char *xcb_xrm_entry_to_string(xcb_xrm_entry_t *entry) { | |
} | |
escaped_value = xcb_xrm_entry_escape_value(entry->value); | |
- sasprintf(&value_buf, "%s: %s", result, escaped_value); | |
+ if (sasprintf(&value_buf, "%s: %s", result, escaped_value) < 0) { | |
+ FREE(escaped_value); | |
+ FREE(result); | |
+ return NULL; | |
+ } | |
FREE(escaped_value); | |
FREE(result); | |
result = value_buf; | |
@@ -348,33 +358,26 @@ char *xcb_xrm_entry_to_string(xcb_xrm_entry_t *entry) { | |
* | |
*/ | |
char *xcb_xrm_entry_escape_value(const char *value) { | |
- char *copy; | |
char *escaped; | |
char *outwalk; | |
int new_size = strlen(value) + 1; | |
- copy = sstrdup(value); | |
- if (copy == NULL) | |
- return NULL; | |
- | |
- if (copy[0] == ' ' || copy[0] == '\t') | |
+ if (value[0] == ' ' || value[0] == '\t') | |
new_size++; | |
- for (char *walk = copy; *walk != '\0'; walk++) { | |
+ for (const char *walk = value; *walk != '\0'; walk++) { | |
if (*walk == '\n' || *walk == '\\') | |
new_size++; | |
} | |
escaped = scalloc(1, new_size); | |
- if (escaped == NULL) { | |
- FREE(copy); | |
+ if (escaped == NULL) | |
return NULL; | |
- } | |
outwalk = escaped; | |
- if (copy[0] == ' ' || copy[0] == '\t') { | |
+ if (value[0] == ' ' || value[0] == '\t') { | |
*(outwalk++) = '\\'; | |
} | |
- for (char *walk = copy; *walk != '\0'; walk++) { | |
+ for (const char *walk = value; *walk != '\0'; walk++) { | |
if (*walk == '\n') { | |
*(outwalk++) = '\\'; | |
*(outwalk++) = 'n'; | |
@@ -386,7 +389,6 @@ char *xcb_xrm_entry_escape_value(const char *value) { | |
} | |
} | |
*outwalk = '\0'; | |
- FREE(copy); | |
return escaped; | |
} | |
diff --git a/src/match.c b/src/match.c | |
index 307daf3..cbf618b 100644 | |
--- a/src/match.c | |
+++ b/src/match.c | |
@@ -79,8 +79,10 @@ int xcb_xrm_match(xcb_xrm_database_t *database, xcb_xrm_entry_t *query_name, xcb | |
if (best_match != NULL) { | |
resource->value = sstrdup(best_match->entry->value); | |
- if (resource->value == NULL) | |
+ if (resource->value == NULL) { | |
+ __match_free(best_match); | |
return -FAILURE; | |
+ } | |
__match_free(best_match); | |
return SUCCESS; | |
@@ -149,6 +151,7 @@ static int __match_matches(xcb_xrm_entry_t *db_entry, xcb_xrm_entry_t *query_nam | |
break; | |
default: | |
/* Never reached. */ | |
+ assert(0); | |
return -FAILURE; | |
} | |
} | |
diff --git a/src/util.c b/src/util.c | |
index 6fdc50a..f6f0f89 100644 | |
--- a/src/util.c | |
+++ b/src/util.c | |
@@ -32,10 +32,12 @@ | |
char *sstrdup(const char *str) { | |
return strdup(str); | |
+ /* Remove, it's unnecessary since err() isn't used any more */ | |
} | |
void *scalloc(size_t num, size_t size) { | |
return calloc(num, size); | |
+ /* Remove */ | |
} | |
int sasprintf(char **strp, const char *fmt, ...) { | |
@@ -45,6 +47,7 @@ int sasprintf(char **strp, const char *fmt, ...) { | |
va_start(args, fmt); | |
result = vasprintf(strp, fmt, args); | |
va_end(args); | |
+ /* Remove */ | |
return result; | |
} | |
@@ -85,7 +88,7 @@ char *file_get_contents(const char *filename) { | |
file_size = stbuf.st_size; | |
/* Read the file content. */ | |
- content = scalloc(file_size, 1); | |
+ content = scalloc(file_size + 1, 1); | |
if (content == NULL) { | |
fclose(file); | |
return NULL; | |
diff --git a/tests/test.c b/tests/test.c | |
index f7b9d17..53d95c9 100644 | |
--- a/tests/test.c | |
+++ b/tests/test.c | |
@@ -416,7 +416,7 @@ static void setup(void) { | |
} | |
static void cleanup(void) { | |
- xcb_disconnect(conn); | |
+ XCloseDisplay(display); | |
} | |
static int check_parse_entry(const char *str, const char *value, const char *bindings, const int count, ...) { |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment