Skip to content

Commit

Permalink
Fix a crash due to an out-of-bounds array access
Browse files Browse the repository at this point in the history 
entry_search had a "type confusion" bug where it would perform array
accesses that bypassed bounds checks if given arguments that looked like
strategies. I've rewritten most of the argument parsing loop in a way
that should eliminate this issue, while being shorter and more
straightforward (with less duplicated code), only requiring one pass
through the arguments. The downside is that we allocate a bit more
memory and have a couple of gotos to handle errors.
  • Loading branch information
@saagarjha @jmroot
saagarjha authored and jmroot committed Feb 17, 2019
1 parent fda5fea commit 4eb0095
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 101 deletions.
4 changes: 2 additions & 2 deletions src/cregistry/entry.c
View file
Expand Up @@ -397,8 +397,8 @@ static int reg_all_entries(reg_registry* reg, char* query, int query_len,
* @param [out] errPtr on error, a description of the error that occurred
* @return the number of entries if success; false if failure
*/
int reg_entry_search(reg_registry* reg, char** keys, char** vals, int key_count,
int *strategies, reg_entry*** entries, reg_error* errPtr) {
int reg_entry_search(reg_registry* reg, const char** keys, const char** vals,
int key_count, int *strategies, reg_entry*** entries, reg_error* errPtr) {
int i;
char* kwd = " WHERE ";
char* query;
Expand Down
4 changes: 2 additions & 2 deletions src/cregistry/entry.h
View file
Expand Up @@ -52,8 +52,8 @@ int reg_entry_delete(reg_entry* entry, reg_error* errPtr);

void reg_entry_free(reg_entry* entry);

int reg_entry_search(reg_registry* reg, char** keys, char** vals, int key_count,
int* strategies, reg_entry*** entries, reg_error* errPtr);
int reg_entry_search(reg_registry* reg, const char** keys, const char** vals,
int key_count, int* strategies, reg_entry*** entries, reg_error* errPtr);

int reg_entry_imaged(reg_registry* reg, const char* name, const char* version,
const char* revision, const char* variants, reg_entry*** entries,
Expand Down
170 changes: 73 additions & 97 deletions src/registry2.0/entry.c
View file
Expand Up @@ -248,116 +248,92 @@ static strategy_type strategies[] = {
* Can be given an option of -exact, -glob, -regexp or -null to specify the
* matching strategy; defaults to exact.
*/
static int entry_search(Tcl_Interp* interp, int objc, Tcl_Obj* CONST objv[]) {
int i, j;
reg_registry* reg = registry_for(interp, reg_attached);
if (reg == NULL) {
static int entry_search(Tcl_Interp* interp, int objc, Tcl_Obj* CONST *objv) {
reg_registry *reg = registry_for(interp, reg_attached);
if (!reg) {
return TCL_ERROR;
} else {
char** keys;
char** vals;
int* strats;
int key_count = 0;
reg_entry** entries;
reg_error error;
int entry_count;
for (i = 2; i < objc;) {
int index, strat_index, val_length;
if (Tcl_GetIndexFromObj(interp, objv[i], entry_props, "search key",
0, &index) != TCL_OK) {
return TCL_ERROR;
}

/* we ate the key value */
i++;

/* check whether there's a strategy */
if (Tcl_GetString(objv[i])[0] == '-'
&& Tcl_GetIndexFromObjStruct(interp, objv[i], strategies,
sizeof(strategy_type), "option", 0, &strat_index)
!= TCL_ERROR) {
/* this key has a strategy specified, eat the strategy parameter */
i++;

if (strategies[strat_index].strategy != reg_strategy_null) {
/* this key must also have a value */

if (Tcl_GetStringFromObj(objv[i], &val_length) == NULL) {
Tcl_WrongNumArgs(interp, 2, objv,
"search ?key ?options? value ...?");
return TCL_ERROR;
}

i++;
}
} else {
/* this key must also have a value */
}
const char **keys = malloc(objc * sizeof(char *));
const char **vals = malloc(objc * sizeof(char *));
int *strats = malloc(objc * sizeof(int));
if (!keys || !vals || !strats) {
goto cleanup_error;
}
int key_count = 0;

if (Tcl_GetStringFromObj(objv[i], &val_length) == NULL) {
Tcl_WrongNumArgs(interp, 2, objv,
"search ?key ?options? value ...?");
return TCL_ERROR;
}
/* Skip the "registry::entry" "search" bit */
objv += 2;
objc -= 2;

i++;
}
for (int i = 0; i < objc; ++key_count /* i in incremented in the loop */) {
int key_index, strategy_index;

key_count++;
/* Grab the key */
if (Tcl_GetIndexFromObj(interp, objv[i], entry_props, "search key",
0, &key_index) != TCL_OK) {
goto cleanup_error;
}

keys = malloc(key_count * sizeof(char*));
vals = malloc(key_count * sizeof(char*));
strats = malloc(key_count * sizeof(int));
if (!keys || !vals || !strats) {
return TCL_ERROR;
keys[key_count] = entry_props[key_index];
if (objc <= ++i) {
goto wrong_num_args; /* We're missing a strategy or value */
}
for (i = 2, j = 0; i < objc && j < key_count; j++) {
int strat_index;

keys[j] = Tcl_GetString(objv[i++]);

/* try to get the strategy */
if (Tcl_GetString(objv[i])[0] == '-'
&& Tcl_GetIndexFromObjStruct(interp, objv[i], strategies,
sizeof(strategy_type), "option", 0, &strat_index)
!= TCL_ERROR) {
/* this key has a strategy specified */
i++;

strats[j] = strategies[strat_index].strategy;
} else {
/* use default strategy */
strats[j] = reg_strategy_exact;
/* Set the default strategy, in case we don't find one */
strats[key_count] = reg_strategy_exact;
/* Try to grab the strategy */
if (Tcl_GetIndexFromObjStruct(interp, objv[i], strategies,
sizeof(strategy_type), "option", 0, &strategy_index) !=
TCL_ERROR) {
strats[key_count] = strategies[strategy_index].strategy;
if (objc <= ++i && strats[key_count] != reg_strategy_null) {
goto wrong_num_args;
} else if (strats[key_count] == reg_strategy_null) {
/* This doesn't take a value, so start from the top */
continue;
}
}

if (strats[j] != reg_strategy_null) {
vals[j] = Tcl_GetString(objv[i++]);
} else {
vals[j] = NULL;
}
/* Grab the value */
if (!(vals[key_count] = Tcl_GetString(objv[i++]))) {
goto wrong_num_args; /* We're missing a value */
}
entry_count = reg_entry_search(reg, keys, vals, key_count,
strats, &entries, &error);
free(keys);
free(vals);
free(strats);
if (entry_count >= 0) {
int retval;
Tcl_Obj* resultObj;
Tcl_Obj** objs;
if (list_entry_to_obj(interp, &objs, entries, entry_count, &error)){
resultObj = Tcl_NewListObj(entry_count, objs);
Tcl_SetObjResult(interp, resultObj);
free(objs);
retval = TCL_OK;
} else {
retval = registry_failed(interp, &error);
}
free(entries);
return retval;
}

reg_entry **entries;
reg_error error;
int entry_count;
entry_count = reg_entry_search(reg, keys, vals, key_count, strats, &entries,
&error);
free(keys);
free(vals);
free(strats);

if (entry_count >= 0) {
int retval;
Tcl_Obj* resultObj;
Tcl_Obj** objs;
if (list_entry_to_obj(interp, &objs, entries, entry_count, &error)){
resultObj = Tcl_NewListObj(entry_count, objs);
Tcl_SetObjResult(interp, resultObj);
free(objs);
retval = TCL_OK;
} else {
retval = registry_failed(interp, &error);
}
free(entries);
return retval;
} else {
return registry_failed(interp, &error);
}

wrong_num_args:
Tcl_WrongNumArgs(interp, 2, objv,
"search ?key ?options? value ...?");
cleanup_error:
free(keys);
free(vals);
free(strats);
return TCL_ERROR;
}

/*
Expand Down
3 changes: 3 additions & 0 deletions src/registry2.0/tests/entry.tcl
View file
Expand Up @@ -74,6 +74,9 @@ proc main {pextlibname} {
test_set {[registry::entry search name -glob vi*]} {$vim1 $vim2 $vim3}
test_set {[registry::entry search name -regexp {zlib|pcre}]} \
{$zlib $pcre}

# test that passing in confusing arguments doesn't crash
test {[catch {registry::entry search name vim1 --}] == 1}
}

# try mapping files and checking their owners
Expand Down

0 comments on commit 4eb0095

Please sign in to comment.