Re: [patch] courier-imap-4.13 imapd patch replacing malloc, strcat and strcpy with asprintf
That's it in a nutshell, essentially.
I'll take a stab at it, until I get frustrated.
Perhaps my time would be better suited to something else which could
help OpenBSD. For the time being, however, I'll give this a shot.
On 04/20/14 02:18, Maxime Villard wrote:
Le 20/04/2014 02:38, Peter Malone a écrit :
Hi,
I'm using OpenBSD 5.5. courier-imap-4.13 is in the ports tree and it's
quite a mess. I started looking at it today with the hope of just
replacing some of the malloc,strcat & strcpy calls with asprintf, but it
became clear before long that there's lots more issues with this code.
Regardless, here's a patch which fixes some of the malloc,strcat &
strcpy spaghetti in imapd.c
I figure you guys are fairly busy with OpenSSL right now so if it's OK
with you I'll get working on the rest of this code.
I spoke with the courier-imap team and their response was less than
satisfactory - claiming asprintf code won't compile on non-linux
platforms.
courier-imap is *totally buggy*. Not only because there are big
programming mistakes - at a time I started to fix many issues like
double-free, use-after-free, leaks, etc., before letting down -,
but also because this "team" doesn't give a fuck at all.
Give a quick glance at the code: it is just a great mess, with bugs
everywhere, no indentation, and probably no review. Clearly, there
are poor - not to say *no* - considerations for security. That's the
idea I made myself of this project, and you seem to have come to the
same conclusion.
--- imapd_orig.c Sat Apr 19 19:38:18 2014
+++ imapd.c Sat Apr 19 20:09:48 2014
@@ -343,9 +343,8 @@
if (q)
{
- r=malloc(strlen(q)+sizeof("/."));
- if (!r) write_error_exit(0);
- strcat(strcpy(r, q), "/.");
+ if(asprintf(&r, "%s%s", q, "/.") == -1)
+ write_error_exit(0);
if (access(r, 0) == 0)
{
free(r);
@@ -1373,11 +1372,9 @@
static char *alloc_filename(const char *mbox, const char *name)
{
-char *p=malloc(strlen(mbox)+strlen(name)+sizeof("/cur/"));
-
- if (!p) write_error_exit(0);
-
- strcat(strcat(strcpy(p, mbox), "/cur/"), name);
+ char *p;
+ if(asprintf(&p, "%s%s%s", mbox, "/cur/", name) == -1)
+ write_error_exit(0);
return (p);
}
@@ -1812,14 +1809,12 @@
{
#if EXPLICITDIRSYNC
- char *p=malloc(strlen(folder)+sizeof("/new"));
+ char *p;
int fd;
-
- if (!p)
+
+ if(asprintf(&p, "%s%s", folder, "/new") == -1)
write_error_exit(0);
- p=strcat(strcpy(p, folder), "/new");
-
fd=open(p, O_RDONLY);
if (fd >= 0)
@@ -1828,7 +1823,8 @@
close(fd);
}
- p=strcat(strcpy(p, folder), "/cur");
+ if(asprintf(&p, "%s%s", folder, "/cur") == -1)
+ write_error_exit(0);
fd=open(p, O_RDONLY);
@@ -2212,12 +2208,10 @@
{
struct imapscaninfo minfo;
- char *p=malloc(strlen(new_path)+sizeof("/" IMAPDB));
+ char *p;
+ if(asprintf(&p, "%s%s", new_path, "/" IMAPDB) == -1)
+write_error_exit(0);
- if (!p)
- write_error_exit(0);
-
- strcat(strcpy(p, new_path), "/" IMAPDB);
unlink(p);
free(p);
imapscan_init(&minfo);
@@ -2448,14 +2442,12 @@
}
return 0;
}
- q=malloc(strlen(p)+sizeof("/new"));
- if (!q)
+ if(asprintf(&q, "%s%s", p, "/new") == -1)
{
free(p);
maildir_aclt_list_destroy(aclt_list);
return -1;
}
- strcat(strcpy(q, p), "/new");
free(p);
if (maildir_aclt_list_add(aclt_list,
@@ -4216,11 +4208,10 @@
if (p && (p=maildir_shareddir(".", p+1)) != NULL)
{
int rc;
- char *q=malloc(strlen(p)+sizeof("/shared"));
-
- if (!q) write_error_exit(0);
-
- strcat(strcpy(q, p), "/shared");
+ char *q;
+
+ if(asprintf(&q, "%s%s", p, "/shared") == -1)
+ write_error_exit(0);
free(p);
rc=append(tag, tok->tokenbuf, q);
free(q);
@@ -6041,10 +6032,9 @@
isshared=0;
if (is_sharedsubdir(cqinfo.destmailbox))
{
- char *p=malloc(strlen(cqinfo.destmailbox)+sizeof("/shared"));
-
- if (!p) write_error_exit(0);
- strcat(strcpy(p, cqinfo.destmailbox), "/shared");
+ char *p;
+ if(asprintf(&p, "%s%s", cqinfo.destmailbox, "/shared") == -1)
+ write_error_exit(0);
free(mailbox);
mailbox=cqinfo.destmailbox=p;
Re: [patch] courier-imap-4.13 imapd patch replacing malloc, strcat and strcpy with asprintf
Le 20/04/2014 02:38, Peter Malone a écrit :
> Hi,
>
> I'm using OpenBSD 5.5. courier-imap-4.13 is in the ports tree and it's
> quite a mess. I started looking at it today with the hope of just
> replacing some of the malloc,strcat & strcpy calls with asprintf, but it
> became clear before long that there's lots more issues with this code.
>
> Regardless, here's a patch which fixes some of the malloc,strcat &
> strcpy spaghetti in imapd.c
>
> I figure you guys are fairly busy with OpenSSL right now so if it's OK
> with you I'll get working on the rest of this code.
>
> I spoke with the courier-imap team and their response was less than
> satisfactory - claiming asprintf code won't compile on non-linux
> platforms.
courier-imap is *totally buggy*. Not only because there are big
programming mistakes - at a time I started to fix many issues like
double-free, use-after-free, leaks, etc., before letting down -,
but also because this "team" doesn't give a fuck at all.
Give a quick glance at the code: it is just a great mess, with bugs
everywhere, no indentation, and probably no review. Clearly, there
are poor - not to say *no* - considerations for security. That's the
idea I made myself of this project, and you seem to have come to the
same conclusion.
>
> --- imapd_orig.c Sat Apr 19 19:38:18 2014
> +++ imapd.c Sat Apr 19 20:09:48 2014
> @@ -343,9 +343,8 @@
>
> if (q)
> {
> - r=malloc(strlen(q)+sizeof("/."));
> - if (!r) write_error_exit(0);
> - strcat(strcpy(r, q), "/.");
> + if(asprintf(&r, "%s%s", q, "/.") == -1)
> + write_error_exit(0);
> if (access(r, 0) == 0)
> {
> free(r);
> @@ -1373,11 +1372,9 @@
>
> static char *alloc_filename(const char *mbox, const char *name)
> {
> -char *p=malloc(strlen(mbox)+strlen(name)+sizeof("/cur/"));
> -
> - if (!p) write_error_exit(0);
> -
> - strcat(strcat(strcpy(p, mbox), "/cur/"), name);
> + char *p;
> + if(asprintf(&p, "%s%s%s", mbox, "/cur/", name) == -1)
> + write_error_exit(0);
> return (p);
> }
>
> @@ -1812,14 +1809,12 @@
> {
> #if EXPLICITDIRSYNC
>
> - char *p=malloc(strlen(folder)+sizeof("/new"));
> + char *p;
> int fd;
> -
> - if (!p)
> +
> + if(asprintf(&p, "%s%s", folder, "/new") == -1)
> write_error_exit(0);
>
> - p=strcat(strcpy(p, folder), "/new");
> -
> fd=open(p, O_RDONLY);
>
> if (fd >= 0)
> @@ -1828,7 +1823,8 @@
> close(fd);
> }
>
> - p=strcat(strcpy(p, folder), "/cur");
> + if(asprintf(&p, "%s%s", folder, "/cur") == -1)
> + write_error_exit(0);
>
> fd=open(p, O_RDONLY);
>
> @@ -2212,12 +2208,10 @@
> {
> struct imapscaninfo minfo;
>
> - char *p=malloc(strlen(new_path)+sizeof("/" IMAPDB));
> + char *p;
> + if(asprintf(&p, "%s%s", new_path, "/" IMAPDB) == -1)
> +write_error_exit(0);
>
> - if (!p)
> - write_error_exit(0);
> -
> - strcat(strcpy(p, new_path), "/" IMAPDB);
> unlink(p);
> free(p);
> imapscan_init(&minfo);
> @@ -2448,14 +2442,12 @@
> }
> return 0;
> }
> - q=malloc(strlen(p)+sizeof("/new"));
> - if (!q)
> + if(asprintf(&q, "%s%s", p, "/new") == -1)
> {
> free(p);
> maildir_aclt_list_destroy(aclt_list);
> return -1;
> }
> - strcat(strcpy(q, p), "/new");
> free(p);
>
> if (maildir_aclt_list_add(aclt_list,
> @@ -4216,11 +4208,10 @@
> if (p && (p=maildir_shareddir(".", p+1)) != NULL)
> {
> int rc;
> - char *q=malloc(strlen(p)+sizeof("/shared"));
> -
> - if (!q) write_error_exit(0);
> -
> - strcat(strcpy(q, p), "/shared");
> + char *q;
> +
> + if(asprintf(&q, "%s%s", p, "/shared") == -1)
> + write_error_exit(0);
> free(p);
> rc=append(tag, tok->tokenbuf, q);
> free(q);
> @@ -6041,10 +6032,9 @@
> isshared=0;
> if (is_sharedsubdir(cqinfo.destmailbox))
> {
> - char *p=malloc(strlen(cqinfo.destmailbox)+sizeof("/shared"));
> -
> - if (!p) write_error_exit(0);
> - strcat(strcpy(p, cqinfo.destmailbox), "/shared");
> + char *p;
> + if(asprintf(&p, "%s%s", cqinfo.destmailbox, "/shared") == -1)
> + write_error_exit(0);
>
> free(mailbox);
> mailbox=cqinfo.destmailbox=p;
>
Re: [patch] courier-imap-4.13 imapd patch replacing malloc, strcat and strcpy with asprintf
On 2014/04/19 20:38, Peter Malone wrote:
> Hi,
>
> I'm using OpenBSD 5.5. courier-imap-4.13 is in the ports tree and it's
> quite a mess. I started looking at it today with the hope of just
> replacing some of the malloc,strcat & strcpy calls with asprintf, but it
> became clear before long that there's lots more issues with this code.
>
> Regardless, here's a patch which fixes some of the malloc,strcat &
> strcpy spaghetti in imapd.c
>
> I figure you guys are fairly busy with OpenSSL right now so if it's OK
> with you I'll get working on the rest of this code.
>
> I spoke with the courier-imap team and their response was less than
> satisfactory - claiming asprintf code won't compile on non-linux
> platforms.
First off, wrong mailing list, tech@ is not for ports discussions so
please follow-up there if you need to.
This is not the sort of thing we can reasonably handle in patches in
ports. Other mail daemons are available...
> --- imapd_orig.c Sat Apr 19 19:38:18 2014
> +++ imapd.c Sat Apr 19 20:09:48 2014
> @@ -343,9 +343,8 @@
>
> if (q)
> {
> - r=malloc(strlen(q)+sizeof("/."));
> - if (!r) write_error_exit(0);
> - strcat(strcpy(r, q), "/.");
> + if(asprintf(&r, "%s%s", q, "/.") == -1)
> + write_error_exit(0);
> if (access(r, 0) == 0)
> {
> free(r);
> @@ -1373,11 +1372,9 @@
>
> static char *alloc_filename(const char *mbox, const char *name)
> {
> -char *p=malloc(strlen(mbox)+strlen(name)+sizeof("/cur/"));
> -
> - if (!p) write_error_exit(0);
> -
> - strcat(strcat(strcpy(p, mbox), "/cur/"), name);
> + char *p;
> + if(asprintf(&p, "%s%s%s", mbox, "/cur/", name) == -1)
> + write_error_exit(0);
> return (p);
> }
>
> @@ -1812,14 +1809,12 @@
> {
> #if EXPLICITDIRSYNC
>
> - char *p=malloc(strlen(folder)+sizeof("/new"));
> + char *p;
> int fd;
> -
> - if (!p)
> +
> + if(asprintf(&p, "%s%s", folder, "/new") == -1)
> write_error_exit(0);
>
> - p=strcat(strcpy(p, folder), "/new");
> -
> fd=open(p, O_RDONLY);
>
> if (fd >= 0)
> @@ -1828,7 +1823,8 @@
> close(fd);
> }
>
> - p=strcat(strcpy(p, folder), "/cur");
> + if(asprintf(&p, "%s%s", folder, "/cur") == -1)
> + write_error_exit(0);
>
> fd=open(p, O_RDONLY);
>
> @@ -2212,12 +2208,10 @@
> {
> struct imapscaninfo minfo;
>
> - char *p=malloc(strlen(new_path)+sizeof("/" IMAPDB));
> + char *p;
> + if(asprintf(&p, "%s%s", new_path, "/" IMAPDB) == -1)
> +write_error_exit(0);
>
> - if (!p)
> - write_error_exit(0);
> -
> - strcat(strcpy(p, new_path), "/" IMAPDB);
> unlink(p);
> free(p);
> imapscan_init(&minfo);
> @@ -2448,14 +2442,12 @@
> }
> return 0;
> }
> - q=malloc(strlen(p)+sizeof("/new"));
> - if (!q)
> + if(asprintf(&q, "%s%s", p, "/new") == -1)
> {
> free(p);
> maildir_aclt_list_destroy(aclt_list);
> return -1;
> }
> - strcat(strcpy(q, p), "/new");
> free(p);
>
> if (maildir_aclt_list_add(aclt_list,
> @@ -4216,11 +4208,10 @@
> if (p && (p=maildir_shareddir(".", p+1)) != NULL)
> {
> int rc;
> - char *q=malloc(strlen(p)+sizeof("/shared"));
> -
> - if (!q) write_error_exit(0);
> -
> - strcat(strcpy(q, p), "/shared");
> + char *q;
> +
> + if(asprintf(&q, "%s%s", p, "/shared") == -1)
> + write_error_exit(0);
> free(p);
> rc=append(tag, tok->tokenbuf, q);
> free(q);
> @@ -6041,10 +6032,9 @@
> isshared=0;
> if (is_sharedsubdir(cqinfo.destmailbox))
> {
> - char *p=malloc(strlen(cqinfo.destmailbox)+sizeof("/shared"));
> -
> - if (!p) write_error_exit(0);
> - strcat(strcpy(p, cqinfo.destmailbox), "/shared");
> + char *p;
> + if(asprintf(&p, "%s%s", cqinfo.destmailbox, "/shared") == -1)
> + write_error_exit(0);
>
> free(mailbox);
> mailbox=cqinfo.destmailbox=p;
> --- imapd_orig.c Sat Apr 19 19:38:18 2014
> +++ imapd.c Sat Apr 19 20:09:48 2014
> @@ -343,9 +343,8 @@
>
> if (q)
> {
> - r=malloc(strlen(q)+sizeof("/."));
> - if (!r) write_error_exit(0);
> - strcat(strcpy(r, q), "/.");
> + if(asprintf(&r, "%s%s", q, "/.") == -1)
> + write_error_exit(0);
> if (access(r, 0) == 0)
> {
> free(r);
> @@ -1373,11 +1372,9 @@
>
> static char *alloc_filename(const char *mbox, const char *name)
> {
> -char *p=malloc(strlen(mbox)+strlen(name)+sizeof("/cur/"));
> -
> - if (!p) write_error_exit(0);
> -
> - strcat(strcat(strcpy(p, mbox), "/cur/"), name);
> + char *p;
> + if(asprintf(&p, "%s%s%s", mbox, "/cur/", name) == -1)
> + write_error_exit(0);
> return (p);
> }
>
> @@ -1812,14 +1809,12 @@
> {
> #if EXPLICITDIRSYNC
>
> - char *p=malloc(strlen(folder)+sizeof("/new"));
> + char *p;
> int fd;
> -
> - if (!p)
> +
> + if(asprintf(&p, "%s%s", folder, "/new") == -1)
> write_error_exit(0);
>
> - p=strcat(strcpy(p, folder), "/new");
> -
> fd=open(p, O_RDONLY);
>
> if (fd >= 0)
> @@ -1828,7 +1823,8 @@
> close(fd);
[patch] courier-imap-4.13 imapd patch replacing malloc, strcat and strcpy with asprintf
Hi,
I'm using OpenBSD 5.5. courier-imap-4.13 is in the ports tree and it's
quite a mess. I started looking at it today with the hope of just
replacing some of the malloc,strcat & strcpy calls with asprintf, but it
became clear before long that there's lots more issues with this code.
Regardless, here's a patch which fixes some of the malloc,strcat &
strcpy spaghetti in imapd.c
I figure you guys are fairly busy with OpenSSL right now so if it's OK
with you I'll get working on the rest of this code.
I spoke with the courier-imap team and their response was less than
satisfactory - claiming asprintf code won't compile on non-linux
platforms.
--- imapd_orig.c Sat Apr 19 19:38:18 2014
+++ imapd.c Sat Apr 19 20:09:48 2014
@@ -343,9 +343,8 @@
if (q)
{
- r=malloc(strlen(q)+sizeof("/."));
- if (!r) write_error_exit(0);
- strcat(strcpy(r, q), "/.");
+ if(asprintf(&r, "%s%s", q, "/.") == -1)
+ write_error_exit(0);
if (access(r, 0) == 0)
{
free(r);
@@ -1373,11 +1372,9 @@
static char *alloc_filename(const char *mbox, const char *name)
{
-char *p=malloc(strlen(mbox)+strlen(name)+sizeof("/cur/"));
-
- if (!p) write_error_exit(0);
-
- strcat(strcat(strcpy(p, mbox), "/cur/"), name);
+ char *p;
+ if(asprintf(&p, "%s%s%s", mbox, "/cur/", name) == -1)
+ write_error_exit(0);
return (p);
}
@@ -1812,14 +1809,12 @@
{
#if EXPLICITDIRSYNC
- char *p=malloc(strlen(folder)+sizeof("/new"));
+ char *p;
int fd;
-
- if (!p)
+
+ if(asprintf(&p, "%s%s", folder, "/new") == -1)
write_error_exit(0);
- p=strcat(strcpy(p, folder), "/new");
-
fd=open(p, O_RDONLY);
if (fd >= 0)
@@ -1828,7 +1823,8 @@
close(fd);
}
- p=strcat(strcpy(p, folder), "/cur");
+ if(asprintf(&p, "%s%s", folder, "/cur") == -1)
+ write_error_exit(0);
fd=open(p, O_RDONLY);
@@ -2212,12 +2208,10 @@
{
struct imapscaninfo minfo;
- char *p=malloc(strlen(new_path)+sizeof("/" IMAPDB));
+ char *p;
+ if(asprintf(&p, "%s%s", new_path, "/" IMAPDB) == -1)
+write_error_exit(0);
- if (!p)
- write_error_exit(0);
-
- strcat(strcpy(p, new_path), "/" IMAPDB);
unlink(p);
free(p);
imapscan_init(&minfo);
@@ -2448,14 +2442,12 @@
}
return 0;
}
- q=malloc(strlen(p)+sizeof("/new"));
- if (!q)
+ if(asprintf(&q, "%s%s", p, "/new") == -1)
{
free(p);
maildir_aclt_list_destroy(aclt_list);
return -1;
}
- strcat(strcpy(q, p), "/new");
free(p);
if (maildir_aclt_list_add(aclt_list,
@@ -4216,11 +4208,10 @@
if (p && (p=maildir_shareddir(".", p+1)) != NULL)
{
int rc;
- char *q=malloc(strlen(p)+sizeof("/shared"));
-
- if (!q) write_error_exit(0);
-
- strcat(strcpy(q, p), "/shared");
+ char *q;
+
+ if(asprintf(&q, "%s%s", p, "/shared") == -1)
+ write_error_exit(0);
free(p);
rc=append(tag, tok->tokenbuf, q);
free(q);
@@ -6041,10 +6032,9 @@
isshared=0;
if (is_sharedsubdir(cqinfo.destmailbox))
{
- char *p=malloc(strlen(cqinfo.destmailbox)+sizeof("/shared"));
-
- if (!p) write_error_exit(0);
- strcat(strcpy(p, cqinfo.destmailbox), "/shared");
+ char *p;
+ if(asprintf(&p, "%s%s", cqinfo.destmailbox, "/shared") == -1)
+ write_error_exit(0);
free(mailbox);
mailbox=cqinfo.destmailbox=p;
--- imapd_orig.c Sat Apr 19 19:38:18 2014
+++ imapd.c Sat Apr 19 20:09:48 2014
@@ -343,9 +343,8 @@
if (q)
{
- r=malloc(strlen(q)+sizeof("/."));
- if (!r) write_error_exit(0);
- strcat(strcpy(r, q), "/.");
+ if(asprintf(&r, "%s%s", q, "/.") == -1)
+write_error_exit(0);
if (access(r, 0) == 0)
{
free(r);
@@ -1373,11 +1372,9 @@
static char *alloc_filename(const char *mbox, const char *name)
{
-char *p=malloc(strlen(mbox)+strlen(name)+sizeof("/cur/"));
-
- if (!p) write_error_exit(0);
-
- strcat(strcat(strcpy(p, mbox), "/cur/"), name);
+ char *p;
+ if(asprintf(&p, "%s%s%s", mbox, "/cur/", name) == -1)
+ write_error_exit(0);
return (p);
}
@@ -1812,14 +1809,12 @@
{
#if EXPLICITDIRSYNC
- char *p=malloc(strlen(folder)+sizeof("/new"));
+ char *p;
int fd;
-
- if (!p)
+
+ if(asprintf(&p, "%s%s", folder, "/new") == -1)
write_error_exit(0);
- p=strcat(strcpy(p, folder), "/new");
-
fd=open(p, O_RDONLY);
if (fd >= 0)
@@ -1828,7 +1823,8 @@
close(fd);
}
- p=strcat(strcpy(p, folder), "/cur");
+ if(asprintf(&p, "%s%s", folder, "/cur") == -1)
+ write_error_exit(0);
fd=open(p, O_RDONLY);
@@ -2212,12 +2208,10 @@
{
struct imapscaninfo minfo;
- char *p=malloc(strlen(new_path)+sizeof("/" IMAPDB));
+ char *p;
+ if(asprintf(&p, "%s%s", new_path, "/" IMAPDB) == -1)
+write_error_exit(0);
- if (!p)
- write_error_exit(0);
-
- strcat(strcpy(p, new_path), "/" IMAPDB);
unlink(p);
free(p);
imapscan_init(&minfo);
@@ -2448,14 +2442,12 @@
}
return 0;
}
- q=malloc(strlen(p)+sizeof("/new"));
- if (!q)
+ if(asprintf(&q, "%s%s", p, "/new") == -1)
{
free(p);
maildir_aclt_list_destroy(aclt_list);
return -1;
}
- strcat(strcpy(q, p), "/new");
free(p);
if (maildir_aclt_list_add(aclt_list,
@@ -4216,11 +4208,10 @@
if (p && (p=maildir_shareddir(".", p+1)) != NULL)
{
int
