Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Untitled #2

Closed
wants to merge 1 commit into from

2 participants

@diegonc

Hello,

My fork contains some patches and I thought you may be interested in some of them.
Currently, they are required to fix some issues in Arch Hurd.

Special attention should be given to the last hunk of 3bf902a.
There's a marginal chance that an undesired cookie file is used when strndup truncates the file path
given by the user. :D

There are also changes to allow overriding some configuration variable from the envirnment and make
command line. It's quite handy as patches to change the configuration tend to get outdated.

I hope you find this useful :)

@falconindy
Owner

I'll probably pull the 3 trivial patches. I, just by chance, noticed that burp was on Hurd's AUR the other day and thought that was pretty cool.

I'm not sure why you're adding a whole new function just to avoid usage of PATH_MAX. While I agree with not using it (http://insanecoding.blogspot.com/2007/11/pathmax-simply-isnt.html), why not just calculate the components and then allocate the filename buffer with asprintf(3)?

Static analysis of the new function also raises some flags with me.

  • The path being returned is a mix of stack and heap allocation, which inevitably leads to stack corruption on free'ing.
  • There's no reason to be using memcpy in place of memmove.
  • The function returns a char*, so returning 0 doesn't make sense when memory allocation fails. Return NULL.
  • In the case of XDG_CONFIG_HOME not being defined, your buffer isn't big enough. You're guaranteed writing data into heap that isn't yours.

And at the least, the commit message isn't descriptive. Yes, it's avoiding usage of PATH_MAX, but you're adding a whole new function. NAK on this.

@diegonc

Hurd's AUR the other day and thought that was pretty cool.

Burp is awesome. It deserves to be in every AUR out there. :)

I'm not sure why you're adding a whole new function just to avoid usage of PATH_MAX.

It just didn't feel right to put all that inside read_config_file.

why not just calculate the components and then allocate the filename buffer with asprintf(3)?

That's probably a good idea actually.

  • The path being returned is a mix of stack and heap allocation, which inevitably leads to stack corruption on free'ing.

I can't see that. A pointer cannot be "a mix of stack and heap allocation". Besides, the variable named path is returned which is only assigned once (with malloc return value).

  • There's no reason to be using memcpy in place of memmove.

Memory areas are guarenteed to not overlap. Thus, there's actually no reason to use memmove either.

  • The function returns a char*, so returning 0 doesn't make sense when memory allocation fails. Return NULL.
  • In the case of XDG_CONFIG_HOME not being defined, your buffer isn't big enough. You're guaranteed writing data into heap that isn't yours.

Oops, thanks for noting.

Static analysis of the new function also raises some flags with me.

This look useful. What tool are you using?

edit: Sorry. I closed this by mistake :(

@falconindy
Owner

fortunately, i can reopen!

By static analysis I just mean I'm reading and following the code without throwing it at any tools. Tools like clint are way too harsh on my ego. So, its entirely possible that I'm way off. And by way off, I mean that I shouldn't look at code so early in the morning. I'm not seeing whatever it is I thought I was seeing with the mixed allocation (whatever that means, indeed).

That said, there's still too much going on here for my liking. Just a quick example of how you might pare it down:

https://gist.github.com/786931

@diegonc

That said, there's still too much going on here for my liking.

Indeed, I removed all that crap :) and used asprintf. ( 6440449 )

@falconindy falconindy closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 19, 2011
  1. @diegonc

    Don't rely on PATH_MAX.

    diegonc authored
This page is out of date. Refresh to see the latest.
Showing with 49 additions and 9 deletions.
  1. +49 −9 burp.c
View
58 burp.c
@@ -76,24 +76,63 @@ static int category_is_valid(const char *cat) {
return(res ? res->num : 0);
}
+static char* get_config_path() {
+ static char config_file[] = "/burp/burp.conf";
+ static char default_config_prefix[] = "/.config";
+ size_t pathlen;
+ char *xdg_config_home, *path, *ptr;
+ int append_prefix;
+
+ append_prefix = 0;
+ pathlen = sizeof(config_file);
+ xdg_config_home = getenv("XDG_CONFIG_HOME");
+ if (xdg_config_home) {
+ pathlen += strlen(xdg_config_home);
+ } else {
+ xdg_config_home = getenv("HOME");
+ pathlen += sizeof(default_config_prefix) - 1;
+ append_prefix = 1;
+ }
+
+ path = malloc(pathlen);
+ if (!path) {
+ if (config->verbose > 1) {
+ printf("::DEBUG:: Unable to allocate memory.\n");
+ }
+ return(0);
+ }
+
+ ptr = path;
+ strcpy(ptr, xdg_config_home);
+
+ ptr += strlen(xdg_config_home);
+ if (append_prefix) {
+ strcpy(ptr, default_config_prefix);
+ ptr += sizeof(default_config_prefix) - 1;
+ }
+
+ /* copy null terminator, too */
+ memcpy(ptr, config_file, sizeof(config_file));
+ return path;
+}
+
static int read_config_file() {
int ret = 0;
- char *ptr, *xdg_config_home;
- char config_path[PATH_MAX + 1], line[BUFSIZ];
+ char *ptr, *config_path;
+ char line[BUFSIZ];
FILE *conf_fd;
- xdg_config_home = getenv("XDG_CONFIG_HOME");
- if (xdg_config_home) {
- snprintf(&config_path[0], PATH_MAX, "%s/burp/burp.conf", xdg_config_home);
- } else {
- snprintf(&config_path[0], PATH_MAX, "%s/.config/burp/burp.conf",
- getenv("HOME"));
+ config_path = get_config_path();
+ if (! config_path) {
+ /* error already reported. */
+ return(ret);
}
if (! file_exists(config_path)) {
if (config->verbose > 1) {
printf("::DEBUG:: No config file found\n");
}
+ free(config_path);
return(ret);
}
@@ -163,6 +202,7 @@ static int read_config_file() {
}
fclose(conf_fd);
+ free(config_path);
return(ret);
}
@@ -233,7 +273,7 @@ static int parseargs(int argc, char **argv) {
if (config->cookies) {
FREE(config->cookies);
}
- config->cookies = strndup(optarg, PATH_MAX);
+ config->cookies = strdup(optarg);
break;
case 'k':
config->persist = TRUE;
Something went wrong with that request. Please try again.