Re: reviewing OpenSSL's lib/libssl/src/crypto/asn1
On Sun, Apr 20, 2014 at 5:06 PM, Dirk Engling erdge...@erdgeist.org wrote: Dear openbsd devs, I've just put on my rubber gloves to help with your heroic efforts on OpenSSL. I started to dive into OpenSSL's ASN.1 implementation and now wonder how to share my findings, patches and requests without spamming this list. this list is for diffs. post them. keep them reviewable - of meaningful size and doing a certain thing. (as opposed to this diff changes 15 things.. ) Also while scanning through the rest of libssl, I've found an astonishing amount of numeric literals within statements and would like to replace them with meaningful identifiers. What's the policy here? #defines or enums? neither - if you just replace numeric literals it better be obvious you are thinking about it rather than just banally doing stuff like: #define LENGTHIKNOWNOTHINGABOUT 288 + 1 If you're doing that, you're just hiding the horror and making it harder for someone like me to fix it. Short answer - send diffs, expect criticism - not because you suck but because all diffs get criticism. If you can't handle it and improve, don't post.
Correctly document return value of getenv(3)
Clarify the return value of getenv. Index: lib/libc/stdlib/getenv.3 === RCS file: /cvsroot/OpenBSD/src/lib/libc/stdlib/getenv.3,v retrieving revision 1.19 diff -u -p -r1.19 getenv.3 --- lib/libc/stdlib/getenv.35 Jun 2013 03:39:23 - 1.19 +++ lib/libc/stdlib/getenv.321 Apr 2014 01:37:57 - @@ -102,11 +102,13 @@ function deletes all instances of the va .Fa name from the list. .Sh RETURN VALUES -These functions -return zero if successful; otherwise the global variable -.Va errno -is set to indicate the error and \-1 is returned. +.Rv -std putenv setenv unsetenv .Pp +The +.Fn getenv +function returns a pointer to the requested value, or +.Dv NULL +if it could not be found. If .Fn getenv is successful, the string returned should be considered read-only.
Re: reviewing OpenSSL's lib/libssl/src/crypto/asn1
On 21.04.14 01:13, Bob Beck wrote:
this list is for diffs. post them. keep them reviewable - of
meaningful size and doing a certain thing.
(as opposed to this diff changes 15 things.. )
Find attached my patches for some memory leaks, use after frees and some
minor house keeping as follows.
8 8 8 8 8 8
memleak_realloc.patch
* fix memory leak in a2i_ASN1_ENUMERATED, a2i_ASN1_STRING
and a2i_ASN1_INTEGER, in case of goto err
* use realloc instead of s ? realloc(s,l) : malloc(l)
* Files: f_enum.c f_int.c f_string.c
double_free.patch
* remove unnecessary NULL checks before free()
* fix double free in d2i_ASN1_bytes by setting ret-data = NULL
after free, before potential goto err;
* Files: a_bytes.c
calloc_length.patch
* remove unnecessary NULL checks before free()
* use calloc instead of malloc and memset
* remove unnecessary temp variable d
* move loop counter j in for() header
* make calculation of actual length in BN_to_ASN1_ENUMERATED
more transparent
* Files: a_enum.c a_int.c
bad_macros.patch
* remove M_ASN1_New_Malloc, M_ASN1_New, M_ASN1_New_Error marcos,
they hide a malloc and are only used once
* Files: asn1_mac.h x_pkey.c
nullcheck.patch
* remove unnecessary NULL checks before free()
* Files: a_bitstr.c a_gentm.c a_object.c a_utctm.c ameth_lib.c
asn1_gen.c asn_mime.c asn_pack.c bio_asn1.c bio_ndef.c t_x509.c
tasn_dec.c tasn_utl.c x_info.c x_name.c
calloc_realloc.patch
* remove unnecessary NULL checks before free()
* use calloc instead of malloc and memset
* use realloc instead of s ? realloc(s,l) : malloc(l)
* Files: ameth_lib.c asn1_lib.c tasn_new.c
sizeof.patch
* use sizeof(buf) instead of numeric value for passing
to OPENSSL_cleanse
* Files: n_pkey.c
double_null.patch
* remove unnecessary double *pval = NULL in ASN1_primitive_free
* Files: tasn_fre.c
reuse.patch
* remove unnecessary NULL checks before free()
* prevent potential use after free of ret-name in x509_cb
* Files: x_x509.c
8 8 8 8 8 8
Some notes on my first glimpse at the code:
* ASN1_PCTX_new could use a calloc and avoid filling all members
* ASN1_BIT_STRING_set_bit and a2i_ASN1_INTEGER use CRYPTO_realloc_clean
which OPENSSL_cleanse old location, in similar realloc settings,
normal realloc was (mis)used
* still OPENSSL_free inside
* different styles for checking !p vs p == NULL vs if(!(p=malloc()))
* ASN1_STRING_set looks like a bloated and overly complex strdup wrapper
* ASN1_INTEGER_set and ASN1_ENUMERATED_set use some rather peculiar
buffer on the stack which is then copied backwards to allocation
#define LENGTHIKNOWNOTHINGABOUT 288 + 1
If you're doing that, you're just hiding the horror and making it
harder for someone like
me to fix it.
I rather thought about copying constants from the RFC and naming offsets
and lengths into structures as such, also using sizeof(buf) instead of
the hard coded value as in sizeof.patch.
Regards,
erdgeist
Index: asn1_mac.h
===
RCS file: /cvs/src/lib/libssl/src/crypto/asn1/asn1_mac.h,v
retrieving revision 1.10
diff -u -r1.10 asn1_mac.h
--- asn1_mac.h 17 Apr 2014 13:37:48 - 1.10
+++ asn1_mac.h 21 Apr 2014 02:19:24 -
@@ -287,21 +287,6 @@
c.slen-=(c.p-c.q); \
}
-/* New macros */
-#define M_ASN1_New_Malloc(ret,type) \
- if ((ret=(type *)malloc(sizeof(type))) == NULL) \
- { c.line=__LINE__; goto err2; }
-
-#define M_ASN1_New(arg,func) \
- if (((arg)=func()) == NULL) return(NULL)
-
-#define M_ASN1_New_Error(a) \
-/* err:ASN1_MAC_H_err((a),ERR_R_NESTED_ASN1_ERROR,c.line); \
- return(NULL);*/ \
- err2: ASN1_MAC_H_err((a),ERR_R_MALLOC_FAILURE,c.line); \
- return(NULL)
-
-
/* BIG UGLY WARNING! This is so damn ugly I wanna puke. Unfortunately,
some macros that use ASN1_const_CTX still insist on writing in the input
stream. ARGH! ARGH! ARGH! Let's get rid of this macro package.
Index: x_pkey.c
===
RCS file: /cvs/src/lib/libssl/src/crypto/asn1/x_pkey.c,v
retrieving revision 1.10
diff -u -r1.10 x_pkey.c
--- x_pkey.c18 Apr 2014 11:20:32 - 1.10
+++ x_pkey.c21 Apr 2014 02:19:24 -
@@ -109,10 +109,17 @@
X509_PKEY *ret = NULL;
ASN1_CTX c;
- M_ASN1_New_Malloc(ret, X509_PKEY);
+ if ((ret = malloc(sizeof(X509_PKEY))) == NULL )
+ ASN1_MAC_H_err(ASN1_F_X509_PKEY_NEW, ERR_R_MALLOC_FAILURE,
__LINE__ );
+ return NULL;
+ }
ret-version = 0;
- M_ASN1_New(ret-enc_algor, X509_ALGOR_new);
- M_ASN1_New(ret-enc_pkey, M_ASN1_OCTET_STRING_new);
+ ret-enc_algor = X509_ALGOR_new();
+ if (ret-enc_algor == NULL)
+ return NULL;
+ ret-enc_pkey = M_ASN1_OCTET_STRING_new();
+ if (ret-enc_pkey
Re: reviewing OpenSSL's lib/libssl/src/crypto/asn1
On Mon, Apr 21, 2014 at 04:43, Dirk Engling wrote: On 21.04.14 01:13, Bob Beck wrote: this list is for diffs. post them. keep them reviewable - of meaningful size and doing a certain thing. (as opposed to this diff changes 15 things.. ) Find attached my patches for some memory leaks, use after frees and some minor house keeping as follows. memleak_realloc.patch * fix memory leak in a2i_ASN1_ENUMERATED, a2i_ASN1_STRING and a2i_ASN1_INTEGER, in case of goto err * use realloc instead of s ? realloc(s,l) : malloc(l) * Files: f_enum.c f_int.c f_string.c double_free.patch * remove unnecessary NULL checks before free() * fix double free in d2i_ASN1_bytes by setting ret-data = NULL after free, before potential goto err; Thanks. Both of these diffs definitely qualify as doing two things at once. It would be nice to see one diff that changes all the realloc calls, one diff that deletes all the NULL before free, one diff that fixes concrete errors like double free/memory leak, etc. And similarly for all the rest. Break diffs down by function, not file. I noticed three different diffs all removed NULL checks before free. That should be a single diff for all affected files. Also, can you include diffs inline please? One diff per email. Maybe just one or two emails to start, then try sending the rest after we see how that goes?
Re: reviewing OpenSSL's lib/libssl/src/crypto/asn1
On 21.04.14 04:56, Ted Unangst wrote:
Also, can you include diffs inline please? One diff per email. Maybe
just one or two emails to start, then try sending the rest after we
see how that goes?
fix memory leak in a2i_ASN1_ENUMERATED, a2i_ASN1_STRING and
a2i_ASN1_INTEGER, in case of of goto err
Index: f_enum.c
===
RCS file: /cvs/src/lib/libssl/src/crypto/asn1/f_enum.c,v
retrieving revision 1.7
diff -u -r1.7 f_enum.c
--- f_enum.c19 Apr 2014 06:43:34 - 1.7
+++ f_enum.c21 Apr 2014 03:29:56 -
@@ -163,8 +163,6 @@
if (sp == NULL) {
ASN1err(ASN1_F_A2I_ASN1_ENUMERATED,
ERR_R_MALLOC_FAILURE);
- if (s != NULL)
- free(s);
goto err;
}
s = sp;
@@ -196,12 +194,11 @@
}
bs-length = num;
bs-data = s;
- ret = 1;
+ return (1);
-err:
- if (0) {
err_sl:
- ASN1err(ASN1_F_A2I_ASN1_ENUMERATED, ASN1_R_SHORT_LINE);
- }
+ ASN1err(ASN1_F_A2I_ASN1_ENUMERATED, ASN1_R_SHORT_LINE);
+err:
+ free(s);
return (ret);
}
Index: f_int.c
===
RCS file: /cvs/src/lib/libssl/src/crypto/asn1/f_int.c,v
retrieving revision 1.11
diff -u -r1.11 f_int.c
--- f_int.c 19 Apr 2014 06:43:34 - 1.11
+++ f_int.c 21 Apr 2014 03:29:56 -
@@ -167,8 +167,6 @@
if (sp == NULL) {
ASN1err(ASN1_F_A2I_ASN1_INTEGER,
ERR_R_MALLOC_FAILURE);
- if (s != NULL)
- free(s);
goto err;
}
s = sp;
@@ -200,12 +198,11 @@
}
bs-length = num;
bs-data = s;
- ret = 1;
+ return (1);
-err:
- if (0) {
err_sl:
- ASN1err(ASN1_F_A2I_ASN1_INTEGER, ASN1_R_SHORT_LINE);
- }
+ ASN1err(ASN1_F_A2I_ASN1_INTEGER, ASN1_R_SHORT_LINE);
+err:
+ free(s);
return (ret);
}
Index: f_string.c
===
RCS file: /cvs/src/lib/libssl/src/crypto/asn1/f_string.c,v
retrieving revision 1.9
diff -u -r1.9 f_string.c
--- f_string.c 19 Apr 2014 06:43:34 - 1.9
+++ f_string.c 21 Apr 2014 03:29:56 -
@@ -159,8 +159,6 @@
if (sp == NULL) {
ASN1err(ASN1_F_A2I_ASN1_STRING,
ERR_R_MALLOC_FAILURE);
- if (s != NULL)
- free(s);
goto err;
}
s = sp;
@@ -192,12 +190,11 @@
}
bs-length = num;
bs-data = s;
- ret = 1;
+ return (1);
-err:
- if (0) {
err_sl:
- ASN1err(ASN1_F_A2I_ASN1_STRING, ASN1_R_SHORT_LINE);
- }
+ ASN1err(ASN1_F_A2I_ASN1_STRING, ASN1_R_SHORT_LINE);
+err:
+ free(s);
return (ret);
}
Re: reviewing OpenSSL's lib/libssl/src/crypto/asn1
On 21.04.14 04:56, Ted Unangst wrote:
Also, can you include diffs inline please? One diff per email. Maybe
just one or two emails to start, then try sending the rest after we
see how that goes?
fix double free in d2i_ASN1_bytes by setting ret-data = NULL after
free, before potential goto err;
Index: a_bytes.c
===
RCS file: /cvs/src/lib/libssl/src/crypto/asn1/a_bytes.c,v
retrieving revision 1.10
diff -u -r1.10 a_bytes.c
--- a_bytes.c 17 Apr 2014 16:14:15 - 1.10
+++ a_bytes.c 21 Apr 2014 03:35:25 -
@@ -205,6 +205,7 @@
if ((ret-length len) || (ret-data == NULL)) {
if (ret-data != NULL)
free(ret-data);
+ ret-data = NULL;
s = (unsigned char *)malloc((int)len + 1);
if (s == NULL) {
i = ERR_R_MALLOC_FAILURE;
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);
}
- 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(/
Re: sftp upload resume diff
Simplify the diff:
use -a for both upload and download resume support.
This makes it more consistent.
Index: sftp-client.h
===
RCS file: /cvs/src/usr.bin/ssh/sftp-client.h,v
retrieving revision 1.24
diff -u -p -u -p -r1.24 sftp-client.h
--- sftp-client.h 17 Oct 2013 00:30:13 - 1.24
+++ sftp-client.h 20 Apr 2014 12:00:47 -
@@ -120,13 +120,13 @@ int download_dir(struct sftp_conn *, cha
* Upload 'local_path' to 'remote_path'. Preserve permissions and times
* if 'pflag' is set
*/
-int do_upload(struct sftp_conn *, char *, char *, int, int);
+int do_upload(struct sftp_conn *, char *, char *, int, int, int);
/*
* Recursively upload 'local_directory' to 'remote_directory'. Preserve
* times if 'pflag' is set
*/
-int upload_dir(struct sftp_conn *, char *, char *, int, int, int);
+int upload_dir(struct sftp_conn *, char *, char *, int, int, int, int);
/* Concatenate paths, taking care of slashes. Caller must free result. */
char *path_append(char *, char *);
Index: sftp-client.c
===
RCS file: /cvs/src/usr.bin/ssh/sftp-client.c,v
retrieving revision 1.114
diff -u -p -u -p -r1.114 sftp-client.c
--- sftp-client.c 31 Jan 2014 16:39:19 - 1.114
+++ sftp-client.c 20 Apr 2014 12:00:47 -
@@ -1409,7 +1409,7 @@ download_dir(struct sftp_conn *conn, cha
int
do_upload(struct sftp_conn *conn, char *local_path, char *remote_path,
-int preserve_flag, int fsync_flag)
+int preserve_flag, int resume, int fsync_flag)
{
int local_fd;
int status = SSH2_FX_OK;
@@ -1418,7 +1418,7 @@ do_upload(struct sftp_conn *conn, char *
char *handle, *data;
Buffer msg;
struct stat sb;
- Attrib a;
+ Attrib a, *c = NULL;
u_int32_t startid;
u_int32_t ackid;
struct outstanding_ack {
@@ -1456,6 +1456,26 @@ do_upload(struct sftp_conn *conn, char *
if (!preserve_flag)
a.flags = ~SSH2_FILEXFER_ATTR_ACMODTIME;
+ if (resume) {
+ /* Get remote file size if it exists */
+ if ((c = do_stat(conn, remote_path, 0)) == NULL) {
+ close(local_fd);
+ return -1;
+ }
+
+ if ((off_t)c-size = sb.st_size) {
+ error(destination file bigger or same size as
+ source file);
+ close(local_fd);
+ return -1;
+ }
+
+ if (lseek(local_fd, (off_t)c-size, SEEK_SET) == -1) {
+ close(local_fd);
+ return -1;
+ }
+ }
+
buffer_init(msg);
/* Send open request */
@@ -1463,7 +1483,8 @@ do_upload(struct sftp_conn *conn, char *
buffer_put_char(msg, SSH2_FXP_OPEN);
buffer_put_int(msg, id);
buffer_put_cstring(msg, remote_path);
- buffer_put_int(msg, SSH2_FXF_WRITE|SSH2_FXF_CREAT|SSH2_FXF_TRUNC);
+ buffer_put_int(msg, SSH2_FXF_WRITE|SSH2_FXF_CREAT|
+ (resume ? SSH2_FXF_APPEND : SSH2_FXF_TRUNC));
encode_attrib(msg, a);
send_msg(conn, msg);
debug3(Sent message SSH2_FXP_OPEN I:%u P:%s, id, remote_path);
@@ -1482,7 +1503,7 @@ do_upload(struct sftp_conn *conn, char *
data = xmalloc(conn-transfer_buflen);
/* Read from local and write to remote */
- offset = progress_counter = 0;
+ offset = progress_counter = (resume ? c-size : 0);
if (showprogress)
start_progress_meter(local_path, sb.st_size,
progress_counter);
@@ -1596,7 +1617,7 @@ do_upload(struct sftp_conn *conn, char *
static int
upload_dir_internal(struct sftp_conn *conn, char *src, char *dst, int depth,
-int preserve_flag, int print_flag, int fsync_flag)
+int preserve_flag, int print_flag, int resume, int fsync_flag)
{
int ret = 0, status;
DIR *dirp;
@@ -1665,12 +1686,12 @@ upload_dir_internal(struct sftp_conn *co
continue;
if (upload_dir_internal(conn, new_src, new_dst,
- depth + 1, preserve_flag, print_flag,
+ depth + 1, preserve_flag, print_flag, resume,
fsync_flag) == -1)
ret = -1;
} else if (S_ISREG(sb.st_mode)) {
if (do_upload(conn, new_src, new_dst,
- preserve_flag, fsync_flag) == -1) {
+ preserve_flag, resume, fsync_flag) == -1) {
error(Uploading of file %s to %s failed!,
new_src, new_dst);
ret = -1;
@@ -1689,7 +1710,7 @@ upload_dir_internal(struct sftp_conn *co
int
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;
CommIt the Patch
cat /dev/null EOM
Hi all.
One thing that I always miss in CVS is ability to pick up separate
changes from a modified file. So many times doing the
cp-edit-commit-rm-update dance I've tired and started this utility.
In the beginning it was very simple, but now it's more like the
features Mercurial and Git provide.
I'm highly doubt this will go in base or ports tree (although I do
not have anything against it), but maybe someone else will find
this utility useful. I did a few dozens of commits using it and I
think that most critical bugs are squashed. But be my guest and
feel free to find more. :) Any other comments are welcome, too.
Oh, and this mail could be used to run this script without cutting
a single bit from message body: ksh /path/to/this/letter ;)
--
WBR,
Vadim Zhukov
EOM
#!/bin/ksh
set -e
vcs_opts=
unset vcs_opts[0]
while (($# 1)); do
[[ $1 == -?* ]] || break
vcs_opts[${vcs_opts[#]}]=$1
shift
done
debug=false
[[ -n $DEBUG ]] debug=true
function usage {
echo usage: ${0##*/} [file] ... 2
exit 0
}
p_full=
p_actual=
p_hunk=
cleanup() {
if $debug; then
echo FULL DIFF: $p_full
echo ACTUAL DIFF: $p_actual
echo HUNK DIFF: $p_hunk
else
[[ -n $p_full ]]rm -- $p_full
[[ -n $p_actual ]] rm -- $p_actual
[[ -n $p_hunk ]]rm -- $p_hunk
fi
}
p_full=$(mktemp -t cip.full.)
p_actual=$(mktemp -t cip.actual.)
p_hunk=$(mktemp -t cip.hunk.)
commit_all=false
skip_all=false
hunk=
index=
unset hunk_start
last_add_index=
add_hunk() {
if [[ $last_add_index != $index ]]; then
echo --- $index.orig $p_actual
echo +++ $index $p_actual
last_add_index=$index
fi
local l
for l in ${hunk[@]}; do
printf '%s\n' $l $p_actual
done
set -A hunk --
}
parse_hunk_header() {
set -A hunk_start -- $(printf '%s\n' $1 |
perl -ne
'/^@@\s*-([0-9]+)(?:,[0-9]*)?\s*\+([0-9]+)(?:,[0-9]*)?\s*@@/ and print $1
$2\n' ||
true)
((${#hunk_start[@]} == 2))
}
edit_hunk() {
local greeting=Welcome to interactive hunk editor!
local err_line='(unknown)'
local l header_read new_hunk nold nnew
# Loop until no error
while true; do
eval greeting=\${greeting}\
cat $p_hunk EOF
$greeting
Do not worry about counters, but do not remove first line
in a hunk. Everything above @@ line will be ignored.
You can cancel addition of the hunk by clearing everything
below the @@ line.
--- $index.orig
+++ $index
EOF
greeting='Patch hunk is corrupt at line $err_line, please
re-edit.'
for l in ${hunk[@]}; do
printf '%s\n' $l $p_hunk
done
${VISUAL:-vi} -- $p_hunk
set -A new_hunk -- ${hunk[0]}
header_read=false
err_line=0
nold=0
nnew=0
IFS=
while read -r l; do
unset IFS
if ! $header_read; then
parse_hunk_header $l header_read=true
continue
fi
((++err_line))
case $l in
-*)
((++nold))
;;
+*)
((++nnew))
;;
*)
((++nold))
((++nnew))
;;
*)
continue 2
;;
esac
new_hunk[${#new_hunk[@]}]=$l
done $p_hunk
unset IFS
err_line='(unknown)'
$header_read || continue
((${#new_hunk[@]} == 0)) return
new_hunk[0]=@@ -${hunk_start[0]},$nold +${hunk_start[1]},$nnew
@@
# Check if hunk is still applicable.
{
echo Index: $index
echo --- $index.orig
echo +++ $index
for l in ${new_hunk[@]}; do
printf '%s\n' $l
done
} | patch -CR || continue
break
done
set -A hunk -- ${new_hunk[@]}
add_hunk
}
help_for_hunk() {
cat 2 EOF
[Y]es - add this hunk to commit, proceed to next hunk.
[N]o - ignore this hunk in commit, proceed to next hunk.
[A]ll - add this and all remaining hunks to commit.
[P]roceed - ignore this
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;
reviewing OpenSSL's lib/libssl/src/crypto/asn1
Dear openbsd devs, I've just put on my rubber gloves to help with your heroic efforts on OpenSSL. I started to dive into OpenSSL's ASN.1 implementation and now wonder how to share my findings, patches and requests without spamming this list. Also while scanning through the rest of libssl, I've found an astonishing amount of numeric literals within statements and would like to replace them with meaningful identifiers. What's the policy here? #defines or enums? Regards, erdgeist
