Re: malloc in libssl/src/apps
On Mon, 5 May 2014, Jean-Philippe Ouellet wrote:
On Mon, May 05, 2014 at 11:12:00AM +1000, Joel Sing wrote:
- i = 0;
if (arg-count == 0) {
arg-count = 20;
- arg-data = (char **)malloc(sizeof(char *) * arg-count);
+ arg-data = calloc(arg-count, sizeof(char *));
}
- for (i = 0; i arg-count; i++)
- arg-data[i] = NULL;
This one is a change in behaviour - if arg-count is 0 then previously
we zeroed arg-data; now we do not.
This one is calloc, not reallocarray, so unless I'm seriously missing
something obvious here, it is indeed zero'd, no?
Run the following before and after your change:
#include stdio.h
#include strings.h
#include openssl/bio.h
#include openssl/conf.h
#include apps.h
BIO *bio_err;
CONF *config;
int
main(int argc, char **argv)
{
char buf[128] = -one -two -three -four -five;
ARGS args;
int i;
memset(args, 0, sizeof(args));
chopup_args(args, buf, argc, argv);
for (i = 0; i args.count; i++)
printf(%i = %p\n, i, args.data[i]);
strlcpy(buf, -one -two, sizeof(buf));
chopup_args(args, buf, argc, argv);
for (i = 0; i args.count; i++)
printf(%i = %p\n, i, args.data[i]);
}
$ gcc -o chopup chopup.c /usr/src/lib/libssl/src/apps/apps.c -I
/usr/src/lib/libssl/src/apps -lcrypto
--
Action without study is fatal. Study without action is futile.
-- Mary Ritter Beard
Re: malloc in libssl/src/apps
On Mon, May 05, 2014 at 07:31:34PM +1000, Joel Sing wrote: This one is calloc, not reallocarray, so unless I'm seriously missing something obvious here, it is indeed zero'd, no? Run the following before and after your change: Ah, yep. Can't believe I missed that (along with all the other obvious errors). I'm an idiot and this patch is full of shit, please ignore it completely. I'll go back to stuff I actually can do, and I'll be sure to read / test my changes while not half asleep before sending them in the future. Thanks to all for the feedback, and sorry for the noise.
Re: malloc in libssl/src/apps
On 05/06/14 00:10, Matthew Dempsky wrote:
On Sun, May 4, 2014 at 8:26 PM, Jean-Philippe Ouellet
jean-phili...@ouellet.biz wrote:
On Sun, May 04, 2014 at 11:30:40PM +0200, Alexander Hall wrote:
NULL theoretically could be != 0
Umm... short of something like:
#undef NULL
#define NULL I'm silly and want to break everything
or something, I don't see when that'd be the case.
I assumed from context that halex@ was asking about null pointers
having a non-zero memory representation, which is allowed by ISO C and
POSIX. But all OpenBSD platforms guarantee that all-bits-are-zero
pointer values are null pointers.
Yeah, I don't know much about standards, but that sounds like what my
mind was referring to. :-)
I'm almost certain that OpenSSH probably relies on this in places too,
so I think we're fine to rely on it in LibreSSL.
I believe a similar situation could appear with not explicitly
initialized global or static declarations, e.g. in
sbin/fsirand/fsirand.c:
fsirand(char *device)
{
...
static char *inodebuf;
...
if ((ib = realloc(inodebuf, ibufsize)) == NULL)
...
}
Anyway, since people more knowledgeable than me on the topic (meaning
lots of people) do not consider this an issue, I guess it isn't.
/Alexander
Re: malloc in libssl/src/apps
On Mon, May 5, 2014 at 3:56 PM, Alexander Hall alexan...@beard.se wrote:
I believe a similar situation could appear with not explicitly initialized
global or static declarations, e.g. in
sbin/fsirand/fsirand.c:
fsirand(char *device)
{
...
static char *inodebuf;
This is safe too: C requires that pointer variables with static
storage duration like this be initialized to a null pointer (C99
section 6.7.8 paragraph 10), not to a sequence of 0 bytes.
Questions about C (was: Re: malloc in libssl/src/apps)
Matthew Dempsky matt...@dempsky.org writes:
On Mon, May 5, 2014 at 3:56 PM, Alexander Hall alexan...@beard.se wrote:
I believe a similar situation could appear with not explicitly initialized
global or static declarations, e.g. in
sbin/fsirand/fsirand.c:
fsirand(char *device)
{
...
static char *inodebuf;
This is safe too: C requires that pointer variables with static
storage duration like this be initialized to a null pointer (C99
section 6.7.8 paragraph 10), not to a sequence of 0 bytes.
People often ask where to find this kind of information, but the
C99 standard[1] is actually available free of charge, same for C11[2].
I also like to point them at the C FAQ[3].
[1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf
[2] http://www.open-std.org/jtc1/sc22/wg14/www/standards.html
[3] http://c-faq.com/
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: malloc in libssl/src/apps
On May 6, 2014 1:34:01 AM CEST, Matthew Dempsky matt...@dempsky.org wrote:
On Mon, May 5, 2014 at 3:56 PM, Alexander Hall alexan...@beard.se
wrote:
I believe a similar situation could appear with not explicitly
initialized
global or static declarations, e.g. in
sbin/fsirand/fsirand.c:
fsirand(char *device)
{
...
static char *inodebuf;
This is safe too: C requires that pointer variables with static
storage duration like this be initialized to a null pointer (C99
section 6.7.8 paragraph 10), not to a sequence of 0 bytes.
Cool, thanks.
Re: malloc in libssl/src/apps
On Sun, May 04, 2014 at 02:38:40AM -0400, Jean-Philippe Ouellet wrote:
Hello,
I've gone through lib/libssl/src/apps with the goal of making {m,c,re}alloc
uses more idiomatic, adding error checking in some places where missing,
and some minor style unification.
Feedback appreciated, better patches to come after the semester ends.
Index: apps.c
===
RCS file: /cvs/src/lib/libssl/src/apps/apps.c,v
retrieving revision 1.45
diff -u -p -r1.45 apps.c
--- apps.c3 May 2014 16:03:54 - 1.45
+++ apps.c4 May 2014 06:07:45 -
@@ -209,13 +209,12 @@ chopup_args(ARGS * arg, char *buf, int *
*argc = 0;
*argv = NULL;
- i = 0;
if (arg-count == 0) {
arg-count = 20;
- arg-data = (char **)malloc(sizeof(char *) * arg-count);
+ arg-data = calloc(arg-count, sizeof(char *));
+ if (arg-data == NULL)
+ return 0;
}
- for (i = 0; i arg-count; i++)
- arg-data[i] = NULL;
num = 0;
p = buf;
@@ -232,8 +231,7 @@ chopup_args(ARGS * arg, char *buf, int *
if (num = arg-count) {
char **tmp_p;
int tlen = arg-count + 20;
- tmp_p = (char **) realloc(arg-data,
- sizeof(char *) * tlen);
+ tmp_p = reallocarray(arg-data, tlen, sizeof(char *));
if (tmp_p == NULL)
return 0;
arg-data = tmp_p;
@@ -266,7 +264,7 @@ chopup_args(ARGS * arg, char *buf, int *
}
*argc = num;
*argv = arg-data;
- return (1);
+ return 1;
}
int
@@ -404,7 +402,28 @@ password_callback(char *buf, int bufsiz,
ok = UI_add_input_string(ui, prompt, ui_flags, buf,
PW_MIN_LENGTH, bufsiz - 1);
if (ok = 0 verify) {
- buff = (char *) malloc(bufsiz);
+ buff = malloc(bufsiz);
+ /*
+ * NULL returns appear to be handled by the following:
+ *
+ * UI_add_verify_string(x, x, x, buff=NULL, x, x, x) -
+ * general_allocate_string(x, x, x, UIT_VERIFY, x, \
+ * result_buf=NULL, x, x, x) -
+ * general_allocate_prompt(x, x, x, x, x, NULL) -
+ * ret = NULL;
+ * if (type == UIT_VERIFY result_buf == NULL)
+ * UIerr(...); and dont touch ret
+ * returns NULL
+ * returns -1
+ * returns -1
+ *
+ * So, we /should/ (maybe) be good. Not calling UIerr()
+ * could very well have some well-hidden side-effects
+ * that I would definitly not notice myself, so I'll
+ * leave this as is without an explicit check here.
+ * Maybe somebody who knows better than I has a better
+ * suggestion?
+ */
ok = UI_add_verify_string(ui, prompt, ui_flags, buff,
PW_MIN_LENGTH, bufsiz - 1, buf);
}
@@ -1830,26 +1849,30 @@ parse_yesno(const char *str, int def)
X509_NAME *
parse_name(char *subject, long chtype, int multirdn)
{
- size_t buflen = strlen(subject) + 1;/* to copy the types and
- * values into. due to
- * escaping, the copy can
- * only become shorter */
- char *buf = malloc(buflen);
- size_t max_ne = buflen / 2 + 1; /* maximum number of name elements */
- char **ne_types = malloc(max_ne * sizeof(char *));
- char **ne_values = malloc(max_ne * sizeof(char *));
- int *mval = malloc(max_ne * sizeof(int));
+ size_t buflen, max_ne;
+ char **ne_types, **ne_values, *buf, *sp, *bp;
+ int *mval, i, nid, ne_num = 0;
+ X509_NAME *n = NULL;
- char *sp = subject, *bp = buf;
- int i, ne_num = 0;
+ /* Due to escaping, buf can never be bigger than subject. */
+ buflen = strlen(subject + 1);
Fail!
- X509_NAME *n = NULL;
- int nid;
+ /* maximum number of name elements */
+ max_ne = buflen / 2 + 1;
+
+ buf = malloc(buflen);
+ ne_types = malloc(max_ne);
+ ne_values = malloc(max_ne);
+ mval = reallocarray(NULL, max_ne, sizeof(int));
why not use calloc(2)?
if (!buf || !ne_types || !ne_values || !mval) {
BIO_printf(bio_err, malloc error\n);
goto error;
}
+
+ sp
Re: malloc in libssl/src/apps
On Sun, May 4, 2014 at 12:21 AM, patrick keshishian sids...@boxsoft.comwrote:
On Sun, May 04, 2014 at 02:38:40AM -0400, Jean-Philippe Ouellet wrote:
...
- if ((irow = (char **)malloc(sizeof(char *) *
- (DB_NUMBER + 1))) == NULL) {
+ irow = reallocarray(NULL, DB_NUMBER + 1, sizeof(char *));
why not use calloc(2)?
Please justify your belief that the existing code is wrong by lack of
memset/bzero. Please do so for *ALL* the places where you suggested using
calloc() where Jean-Philippe Ouellet's patch suggest reallocarray().
Either:
a) the existing code's lack of memset/bzero is a bug (details?), OR
b) using calloc() instead of reallocarray() is *wrong*.
Philip Guenther
Re: malloc in libssl/src/apps
On Sun, May 04, 2014 at 12:29:59AM -0700, Philip Guenther wrote:
On Sun, May 4, 2014 at 12:21 AM, patrick keshishian
sids...@boxsoft.comwrote:
On Sun, May 04, 2014 at 02:38:40AM -0400, Jean-Philippe Ouellet wrote:
...
- if ((irow = (char **)malloc(sizeof(char *) *
- (DB_NUMBER + 1))) == NULL) {
+ irow = reallocarray(NULL, DB_NUMBER + 1, sizeof(char *));
why not use calloc(2)?
Please justify your belief that the existing code is wrong by lack of
memset/bzero. Please do so for *ALL* the places where you suggested using
calloc() where Jean-Philippe Ouellet's patch suggest reallocarray().
How did you deduce my belief to be such?
The code was changing malloc(n * size) to reallocarray().
Typical correction for this usage has been to convert to
calloc(2), in order to avoid possible integer overflows.
Seeing that reallocarray() appeared in OpenBSD 5.6, I am
sure it is an equally fine interface.
Any concern over the first strlen() change I pointed out?
--patrick
Either:
a) the existing code's lack of memset/bzero is a bug (details?), OR
b) using calloc() instead of reallocarray() is *wrong*.
Philip Guenther
Re: malloc in libssl/src/apps
On Sun, May 04, 2014 at 12:21:04AM -0700, patrick keshishian wrote: why not use calloc(2)? Because it doesn't exist ? (hint: the 2 in calloc(2) is the manual section. There is no calloc system call, therefore you can't be right. See guenther(2) for a more serious answer).
Re: malloc in libssl/src/apps
On Sunday, May 4, 2014, patrick keshishian sids...@boxsoft.com wrote:
On Sun, May 04, 2014 at 12:29:59AM -0700, Philip Guenther wrote:
On Sun, May 4, 2014 at 12:21 AM, patrick keshishian
sids...@boxsoft.comjavascript:;
wrote:
On Sun, May 04, 2014 at 02:38:40AM -0400, Jean-Philippe Ouellet wrote:
...
- if ((irow = (char **)malloc(sizeof(char *) *
- (DB_NUMBER + 1))) == NULL) {
+ irow = reallocarray(NULL, DB_NUMBER + 1, sizeof(char
*));
why not use calloc(2)?
Please justify your belief that the existing code is wrong by lack of
memset/bzero. Please do so for *ALL* the places where you suggested
using
calloc() where Jean-Philippe Ouellet's patch suggest reallocarray().
How did you deduce my belief to be such?
The difference between reallocarray(NULL, x, y) and calloc(x, y) is that
the latter zeros the allocated array. Ergo, your suggestion to use calloc()
instead of reallocarray() indicates a belief that the array should be
zeroed, meaning the existing code is wrong.
The code was changing malloc(n * size) to reallocarray().
Typical correction for this usage has been to convert to
calloc(2), in order to avoid possible integer overflows.
Seeing that reallocarray() appeared in OpenBSD 5.6, I am
sure it is an equally fine interface.
Please *support* the use of reallocarray() in code that would otherwise
have to do the overflow check itself (and possibly screw it up) or use
calloc() (and zero possibly large chunks of memory unnecessarily)!
Philip Guenther
Re: malloc in libssl/src/apps
On Sun, May 04, 2014 at 01:26:18AM -0700, Philip Guenther wrote:
On Sunday, May 4, 2014, patrick keshishian sids...@boxsoft.com wrote:
On Sun, May 04, 2014 at 12:29:59AM -0700, Philip Guenther wrote:
On Sun, May 4, 2014 at 12:21 AM, patrick keshishian
sids...@boxsoft.comjavascript:;
wrote:
On Sun, May 04, 2014 at 02:38:40AM -0400, Jean-Philippe Ouellet wrote:
...
- if ((irow = (char **)malloc(sizeof(char *) *
- (DB_NUMBER + 1))) == NULL) {
+ irow = reallocarray(NULL, DB_NUMBER + 1, sizeof(char
*));
why not use calloc(2)?
Please justify your belief that the existing code is wrong by lack of
memset/bzero. Please do so for *ALL* the places where you suggested
using
calloc() where Jean-Philippe Ouellet's patch suggest reallocarray().
How did you deduce my belief to be such?
The difference between reallocarray(NULL, x, y) and calloc(x, y) is that
the latter zeros the allocated array. Ergo, your suggestion to use calloc()
instead of reallocarray() indicates a belief that the array should be
zeroed, meaning the existing code is wrong.
The code was changing malloc(n * size) to reallocarray().
Typical correction for this usage has been to convert to
calloc(2), in order to avoid possible integer overflows.
Seeing that reallocarray() appeared in OpenBSD 5.6, I am
sure it is an equally fine interface.
Please *support* the use of reallocarray() in code that would otherwise
have to do the overflow check itself (and possibly screw it up) or use
calloc() (and zero possibly large chunks of memory unnecessarily)!
That sentence does not make sense. I have read it multiple
times.
I have no idea what you are driving at, other than diverting
attention away from the strlen() mistake (possibly typo) in
the original suggested patch.
That's the real issue any one serious should care about.
Unless, of course, you want to tell me the following two
statements are identical:
1. strlen(subject) + 1;/* original code */
2. strlen(subject + 1);/* suggested patch */
--patrick
Re: malloc in libssl/src/apps
On Sun, May 04, 2014 at 02:38:40AM -0400, Jean-Philippe Ouellet wrote:
Hello,
I've gone through lib/libssl/src/apps with the goal of making {m,c,re}alloc
uses more idiomatic, adding error checking in some places where missing,
and some minor style unification.
Feedback appreciated, better patches to come after the semester ends.
Index: apps.c
===
RCS file: /cvs/src/lib/libssl/src/apps/apps.c,v
retrieving revision 1.45
diff -u -p -r1.45 apps.c
--- apps.c3 May 2014 16:03:54 - 1.45
+++ apps.c4 May 2014 06:07:45 -
@@ -209,13 +209,12 @@ chopup_args(ARGS * arg, char *buf, int *
*argc = 0;
*argv = NULL;
- i = 0;
if (arg-count == 0) {
arg-count = 20;
- arg-data = (char **)malloc(sizeof(char *) * arg-count);
+ arg-data = calloc(arg-count, sizeof(char *));
+ if (arg-data == NULL)
+ return 0;
}
- for (i = 0; i arg-count; i++)
- arg-data[i] = NULL;
num = 0;
p = buf;
@@ -232,8 +231,7 @@ chopup_args(ARGS * arg, char *buf, int *
if (num = arg-count) {
char **tmp_p;
int tlen = arg-count + 20;
- tmp_p = (char **) realloc(arg-data,
- sizeof(char *) * tlen);
+ tmp_p = reallocarray(arg-data, tlen, sizeof(char *));
if (tmp_p == NULL)
return 0;
arg-data = tmp_p;
@@ -266,7 +264,7 @@ chopup_args(ARGS * arg, char *buf, int *
}
*argc = num;
*argv = arg-data;
- return (1);
+ return 1;
}
int
@@ -404,7 +402,28 @@ password_callback(char *buf, int bufsiz,
ok = UI_add_input_string(ui, prompt, ui_flags, buf,
PW_MIN_LENGTH, bufsiz - 1);
if (ok = 0 verify) {
- buff = (char *) malloc(bufsiz);
+ buff = malloc(bufsiz);
+ /*
+ * NULL returns appear to be handled by the following:
+ *
+ * UI_add_verify_string(x, x, x, buff=NULL, x, x, x) -
+ * general_allocate_string(x, x, x, UIT_VERIFY, x, \
+ * result_buf=NULL, x, x, x) -
+ * general_allocate_prompt(x, x, x, x, x, NULL) -
+ * ret = NULL;
+ * if (type == UIT_VERIFY result_buf == NULL)
+ * UIerr(...); and dont touch ret
+ * returns NULL
+ * returns -1
+ * returns -1
+ *
+ * So, we /should/ (maybe) be good. Not calling UIerr()
+ * could very well have some well-hidden side-effects
+ * that I would definitly not notice myself, so I'll
+ * leave this as is without an explicit check here.
+ * Maybe somebody who knows better than I has a better
+ * suggestion?
+ */
ok = UI_add_verify_string(ui, prompt, ui_flags, buff,
PW_MIN_LENGTH, bufsiz - 1, buf);
}
@@ -1830,26 +1849,30 @@ parse_yesno(const char *str, int def)
X509_NAME *
parse_name(char *subject, long chtype, int multirdn)
{
- size_t buflen = strlen(subject) + 1;/* to copy the types and
- * values into. due to
- * escaping, the copy can
- * only become shorter */
- char *buf = malloc(buflen);
- size_t max_ne = buflen / 2 + 1; /* maximum number of name elements */
- char **ne_types = malloc(max_ne * sizeof(char *));
- char **ne_values = malloc(max_ne * sizeof(char *));
- int *mval = malloc(max_ne * sizeof(int));
+ size_t buflen, max_ne;
+ char **ne_types, **ne_values, *buf, *sp, *bp;
+ int *mval, i, nid, ne_num = 0;
+ X509_NAME *n = NULL;
- char *sp = subject, *bp = buf;
- int i, ne_num = 0;
+ /* Due to escaping, buf can never be bigger than subject. */
+ buflen = strlen(subject + 1);
In addition to already mentioned mistake in above strlen()
- X509_NAME *n = NULL;
- int nid;
+ /* maximum number of name elements */
+ max_ne = buflen / 2 + 1;
+
+ buf = malloc(buflen);
+ ne_types = malloc(max_ne);
+ ne_values = malloc(max_ne);
above two malloc() calls are inconsistent with original
code, which passed 'max_ne * sizeof(char *)' to malloc().
--patrick
Re: malloc in libssl/src/apps
On Sun, May 04, 2014 at 12:17:16PM -0600, Theo de Raadt wrote:
We are going to completely ignore diffs which change multiple idioms
at once.
Okay.
That is how mistakes get made.
Yep, more true than I realized.
Here's a simpler one:
Index: apps.c
===
RCS file: /cvs/src/lib/libssl/src/apps/apps.c,v
retrieving revision 1.45
diff -u -p -r1.45 apps.c
--- apps.c 3 May 2014 16:03:54 - 1.45
+++ apps.c 4 May 2014 19:35:59 -
@@ -209,13 +209,10 @@ chopup_args(ARGS * arg, char *buf, int *
*argc = 0;
*argv = NULL;
- i = 0;
if (arg-count == 0) {
arg-count = 20;
- arg-data = (char **)malloc(sizeof(char *) * arg-count);
+ arg-data = calloc(arg-count, sizeof(char *));
}
- for (i = 0; i arg-count; i++)
- arg-data[i] = NULL;
num = 0;
p = buf;
@@ -232,8 +229,7 @@ chopup_args(ARGS * arg, char *buf, int *
if (num = arg-count) {
char **tmp_p;
int tlen = arg-count + 20;
- tmp_p = (char **) realloc(arg-data,
- sizeof(char *) * tlen);
+ tmp_p = reallocarray(arg-data, tlen, sizeof(char *));
if (tmp_p == NULL)
return 0;
arg-data = tmp_p;
@@ -1836,9 +1832,9 @@ parse_name(char *subject, long chtype, i
* only become shorter */
char *buf = malloc(buflen);
size_t max_ne = buflen / 2 + 1; /* maximum number of name elements */
- char **ne_types = malloc(max_ne * sizeof(char *));
- char **ne_values = malloc(max_ne * sizeof(char *));
- int *mval = malloc(max_ne * sizeof(int));
+ char **ne_types = reallocarray(NULL, max_ne, sizeof(char *));
+ char **ne_values = reallocarray(NULL, max_ne, sizeof(char *));
+ int *mval = reallocarray(NULL, max_ne, sizeof(int));
char *sp = subject, *bp = buf;
int i, ne_num = 0;
Index: ca.c
===
RCS file: /cvs/src/lib/libssl/src/apps/ca.c,v
retrieving revision 1.48
diff -u -p -r1.48 ca.c
--- ca.c2 May 2014 17:06:46 - 1.48
+++ ca.c4 May 2014 19:36:00 -
@@ -2002,8 +2002,8 @@ again2:
row[DB_type][0] = 'V';
row[DB_type][1] = '\0';
- if ((irow = (char **)malloc(sizeof(char *) * (DB_NUMBER + 1))) ==
- NULL) {
+ irow = reallocarray(NULL, DB_NUMBER + 1, sizeof(char *));
+ if (irow == NULL) {
BIO_printf(bio_err, Memory allocation failure\n);
goto err;
}
@@ -2267,8 +2267,8 @@ do_revoke(X509 * x509, CA_DB * db, int t
row[DB_type][0] = 'V';
row[DB_type][1] = '\0';
- if ((irow = (char **)malloc(sizeof(char *) *
- (DB_NUMBER + 1))) == NULL) {
+ irow = reallocarray(NULL, DB_NUMBER + 1, sizeof(char *));
+ if (irow == NULL) {
BIO_printf(bio_err, Memory allocation failure\n);
goto err;
}
Index: ecparam.c
===
RCS file: /cvs/src/lib/libssl/src/apps/ecparam.c,v
retrieving revision 1.10
diff -u -p -r1.10 ecparam.c
--- ecparam.c 24 Apr 2014 12:22:22 - 1.10
+++ ecparam.c 4 May 2014 19:36:00 -
@@ -312,7 +312,7 @@ bad:
crv_len = EC_get_builtin_curves(NULL, 0);
- curves = malloc((int) (sizeof(EC_builtin_curve) * crv_len));
+ curves = reallocarray(NULL, crv_len, sizeof(EC_builtin_curve));
if (curves == NULL)
goto end;
Index: speed.c
===
RCS file: /cvs/src/lib/libssl/src/apps/speed.c,v
retrieving revision 1.38
diff -u -p -r1.38 speed.c
--- speed.c 2 May 2014 17:06:46 - 1.38
+++ speed.c 4 May 2014 19:36:00 -
@@ -2178,7 +2178,7 @@ do_multi(int multi)
int *fds;
static char sep[] = :;
- fds = malloc(multi * sizeof *fds);
+ fds = reallocarray(NULL, multi, sizeof(int));
for (n = 0; n multi; ++n) {
if (pipe(fd) == -1) {
fprintf(stderr, pipe failure\n);
Index: srp.c
===
RCS file: /cvs/src/lib/libssl/src/apps/srp.c,v
retrieving revision 1.10
diff -u -p -r1.10 srp.c
--- srp.c 24 Apr 2014 12:22:22 - 1.10
+++ srp.c 4 May 2014 19:36:00 -
@@ -176,7 +176,8 @@ update_index(CA_DB * db, BIO * bio, char
char **irow;
int i;
- if ((irow = (char **) malloc(sizeof(char *) * (DB_NUMBER + 1))) ==
NULL) {
+ irow = reallocarray(NULL, DB_NUMBER + 1,
Re: malloc in libssl/src/apps
On 05/04/14 21:50, Jean-Philippe Ouellet wrote:
On Sun, May 04, 2014 at 12:17:16PM -0600, Theo de Raadt wrote:
We are going to completely ignore diffs which change multiple idioms
at once.
Okay.
That is how mistakes get made.
Yep, more true than I realized.
Here's a simpler one:
Index: apps.c
===
RCS file: /cvs/src/lib/libssl/src/apps/apps.c,v
retrieving revision 1.45
diff -u -p -r1.45 apps.c
--- apps.c 3 May 2014 16:03:54 - 1.45
+++ apps.c 4 May 2014 19:35:59 -
@@ -209,13 +209,10 @@ chopup_args(ARGS * arg, char *buf, int *
*argc = 0;
*argv = NULL;
- i = 0;
if (arg-count == 0) {
arg-count = 20;
- arg-data = (char **)malloc(sizeof(char *) * arg-count);
+ arg-data = calloc(arg-count, sizeof(char *));
}
- for (i = 0; i arg-count; i++)
- arg-data[i] = NULL;
General question; Given that NULL theoretically could be != 0, and that
we generally compare e.g. the malloc() result to NULL specifically, is
this approach acceptable?
/Alexander
Re: malloc in libssl/src/apps
On Sun, May 4, 2014 at 5:00 PM, Alexander Hall alexan...@beard.se wrote:
On 05/04/14 21:50, Jean-Philippe Ouellet wrote:
On Sun, May 04, 2014 at 12:17:16PM -0600, Theo de Raadt wrote:
We are going to completely ignore diffs which change multiple idioms
at once.
Okay.
That is how mistakes get made.
Yep, more true than I realized.
Here's a simpler one:
Index: apps.c
===
RCS file: /cvs/src/lib/libssl/src/apps/apps.c,v
retrieving revision 1.45
diff -u -p -r1.45 apps.c
--- apps.c 3 May 2014 16:03:54 - 1.45
+++ apps.c 4 May 2014 19:35:59 -
@@ -209,13 +209,10 @@ chopup_args(ARGS * arg, char *buf, int *
*argc = 0;
*argv = NULL;
- i = 0;
if (arg-count == 0) {
arg-count = 20;
- arg-data = (char **)malloc(sizeof(char *) * arg-count);
+ arg-data = calloc(arg-count, sizeof(char *));
}
- for (i = 0; i arg-count; i++)
- arg-data[i] = NULL;
General question; Given that NULL theoretically could be != 0, and that we
generally compare e.g. the malloc() result to NULL specifically, is this
approach acceptable?
The comparisons `NULL == 0` and `malloc() == 0` are *value* comparison.
After the statement `void **p = calloc(1, sizeof(void **));`, the
comparisons `p == NULL` and therefore `p == 0`, which are *identical*,
are not necessarily true.
Yes, to be correct you can't assume the representation is a string of
NULs, but the rest of your post is misleading.
/Alexander
Re: malloc in libssl/src/apps
On Sun, May 04, 2014 at 11:30:40PM +0200, Alexander Hall wrote: NULL theoretically could be != 0 Umm... short of something like: #undef NULL #define NULL I'm silly and want to break everything or something, I don't see when that'd be the case. According to ISO/IEC 9899:1999 TC3 (n1256) and the freely available draft of ISO/IEC 9899:201x (n1570), section 6.3.2.3, paragraph 3: [1] An integer constant expression with the value 0, or such an expression cast to type void *, is called a null pointer constant. If a null pointer constant is converted to a pointer type, the resulting pointer, called a null pointer, is guaranteed to compare unequal to a pointer to any object or function. So while it might not be cast to a pointer, it's still zero. And for what it's worth, POSIX does define it to be (void *)0 [2] The stddef.h header shall define the following macros: NULL Null pointer constant. The macro shall expand to an integer constant expression with the value 0 cast to type void *. [1] http://www.iso-9899.info/n1256.html#6.3.2.3p3 [2] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/stddef.h.html Did I miss something again?
Re: malloc in libssl/src/apps
On Mon, May 05, 2014 at 11:12:00AM +1000, Joel Sing wrote:
- i = 0;
if (arg-count == 0) {
arg-count = 20;
- arg-data = (char **)malloc(sizeof(char *) * arg-count);
+ arg-data = calloc(arg-count, sizeof(char *));
}
- for (i = 0; i arg-count; i++)
- arg-data[i] = NULL;
This one is a change in behaviour - if arg-count is 0 then previously we
zeroed arg-data; now we do not.
This one is calloc, not reallocarray, so unless I'm seriously missing
something obvious here, it is indeed zero'd, no?
Re: malloc in libssl/src/apps
On Sun, May 04, 2014 at 03:50:06PM -0400, Jean-Philippe Ouellet wrote:
On Sun, May 04, 2014 at 12:17:16PM -0600, Theo de Raadt wrote:
We are going to completely ignore diffs which change multiple idioms
at once.
Okay.
That is how mistakes get made.
Yep, more true than I realized.
Here's a simpler one:
[...]
Index: speed.c
===
RCS file: /cvs/src/lib/libssl/src/apps/speed.c,v
retrieving revision 1.38
diff -u -p -r1.38 speed.c
--- speed.c 2 May 2014 17:06:46 - 1.38
+++ speed.c 4 May 2014 19:36:00 -
@@ -2178,7 +2178,7 @@ do_multi(int multi)
int *fds;
static char sep[] = :;
- fds = malloc(multi * sizeof *fds);
+ fds = reallocarray(NULL, multi, sizeof(int));
how about a check for return value here?
similar to other malloc - reallocarray changes.
--patrick
for (n = 0; n multi; ++n) {
if (pipe(fd) == -1) {
fprintf(stderr, pipe failure\n);
Index: srp.c
===
RCS file: /cvs/src/lib/libssl/src/apps/srp.c,v
retrieving revision 1.10
diff -u -p -r1.10 srp.c
--- srp.c 24 Apr 2014 12:22:22 - 1.10
+++ srp.c 4 May 2014 19:36:00 -
@@ -176,7 +176,8 @@ update_index(CA_DB * db, BIO * bio, char
char **irow;
int i;
- if ((irow = (char **) malloc(sizeof(char *) * (DB_NUMBER + 1))) ==
NULL) {
+ irow = reallocarray(NULL, DB_NUMBER + 1, sizeof(char *));
+ if (irow == NULL)
BIO_printf(bio_err, Memory allocation failure\n);
return 0;
}
