You should pull this one instead of gtmanfred's #51

Closed
wants to merge 4 commits into
from

3 participants

@Apsu

Because it's his PR but with less fail and moar win :D

gtmanfred and others added some commits Mar 7, 2013
@gtmanfred gtmanfred don't hardcode the buffer for escaped urls 030b841
@Apsu Apsu Fixed that for you
* Pulled the malloc fail check up for moar faster fails
* Avoided the string duplication
* Avoided the memory leak with not freeing the pre-dup buf
3cdc94c
@falconindy
Owner

This is silly. I'll let you figure out why.

Lulz. Empty string segfault gogo!

Actually, I was thinking that the string is already zero terminated, and in the right place. buf[strlen(buf)-1] can't possibly be anything but a 0 byte.

It is already zero-terminated but it will have a delim at the end, whatever the delimiter happens to be, presumably "/" for URLs. Looks to me like your code was aiming to toss that, and I suspect that's correct. As for buf[strlen(buf)-1], if buf doesn't get strcat'd, it will be buf[-1] which tries to access the preceding byte. That's sure to be a no-good for this heap-allocated buffer.

@vodik vodik and 1 other commented on an outdated diff Mar 8, 2013
@@ -2324,7 +2324,12 @@ void *thread_pool(void *arg) /* {{{ */
static char *url_escape(char *in, int len, const char *delim) /* {{{ */
{
char *tok, *escaped;
- char buf[128] = { 0 };
+ char *buf;
+ if((buf = malloc(strlen(in) * 3)) == NULL) {
@vodik
vodik added a line comment Mar 8, 2013

Shouldn't this be strlen(in) * 3 + 1? Worst possible case there still needs to be room for the \0.

@Apsu
Apsu added a line comment Mar 8, 2013

Was considering that, actually. Makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@falconindy falconindy closed this Apr 10, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment