Re: Question about the xmalloc() et al functions
On Mar 9, 2008, at 10:00 PM, Wayne Davison wrote: On Sun, Mar 09, 2008 at 08:27:25PM -0400, Jeff Johnson wrote: Using xmalloc just opens up a can-of-worms while lusers fuss about non-gcc compiler extension portability. Aha, I had failed to notice that the "? :" bit was a gcc extension. What I don't like is that normal users get some memory functions that are fatal if they use gcc, while all others get non-fatal functions. Perhaps the default should be for all normal compiles to get the non- fatal functions, and then anyone that wanted to use the current gcc defines could set something extra (such as -DFATAL_MEM). Very good point. OTOH, I don't understand why some lusers are permitted vasprintf and strlcpy, while others have to hobble along with alternatives. One other reason I care about this is that I think the various bits of code that are doing a strdup()-like action should be using a strdup()- like call, not each one having their own set of strlen/malloc/strcpy calls. It's clearer and a bit safer. Yup. Way too much C developer time is spent fussing with malloc'ing strings. I'm attaching a patch that adds the extra FATAL_MEM requirement, and then uses xstrdup() in a few places (which would mean that strdup() is being used for normal popt users). I'm still considering switching to xstrdup, likely used inconsistently atm. I was also working on changing some code to use your newly-added stpcpy() function, so I included that work too. Some of the changes are optimizations to avoid a strlen() call (when we can easily compute the length via subtraction) and a fix for a potential memory leak if realloc() returns NULL. Nice. I optimized away a few variables, and rewrote findProgramPath, while adding your patch. See what you think. I sometimes regret later what seems clever atm. Off to waste some more development time malloc'ing strings ;-) 73 de Jeff __ POPT Library http://rpm5.org Developer Communication List [email protected]
Re: Question about the xmalloc() et al functions
On Sun, Mar 09, 2008 at 07:00:52PM -0700, Wayne Davison wrote:
> +} else if (prtshort) {
> + left[0] = '-';
> + left[1] = opt->shortName;
> +} else if (prtlong) {
Oops, there should have been a left[2] = '\0' in there.
..wayne..
__
POPT Library http://rpm5.org
Developer Communication List [email protected]
Re: Question about the xmalloc() et al functions
On Sun, Mar 09, 2008 at 08:27:25PM -0400, Jeff Johnson wrote:
> Using xmalloc just opens up a can-of-worms while lusers fuss about
> non-gcc compiler extension portability.
Aha, I had failed to notice that the "? :" bit was a gcc extension.
What I don't like is that normal users get some memory functions that
are fatal if they use gcc, while all others get non-fatal functions.
Perhaps the default should be for all normal compiles to get the non-
fatal functions, and then anyone that wanted to use the current gcc
defines could set something extra (such as -DFATAL_MEM).
One other reason I care about this is that I think the various bits of
code that are doing a strdup()-like action should be using a strdup()-
like call, not each one having their own set of strlen/malloc/strcpy
calls. It's clearer and a bit safer.
I'm attaching a patch that adds the extra FATAL_MEM requirement, and
then uses xstrdup() in a few places (which would mean that strdup() is
being used for normal popt users).
I was also working on changing some code to use your newly-added
stpcpy() function, so I included that work too. Some of the changes
are optimizations to avoid a strlen() call (when we can easily compute
the length via subtraction) and a fix for a potential memory leak if
realloc() returns NULL.
..wayne..
--- system.h9 Mar 2008 20:24:45 - 1.14
+++ system.h10 Mar 2008 01:40:23 -
@@ -78,7 +78,7 @@ static inline char * stpcpy (char *dest,
#endif
/* Memory allocation via macro defs to get meaningful locations from mtrace()
*/
-#if defined(HAVE_MCHECK_H) && defined(__GNUC__)
+#if defined(FATAL_MEM) && defined(HAVE_MCHECK_H) && defined(__GNUC__)
#definevmefail() (fprintf(stderr, "virtual memory
exhausted.\n"), exit(EXIT_FAILURE), NULL)
#definexmalloc(_size) (malloc(_size) ? : vmefail())
#definexcalloc(_nmemb, _size) (calloc((_nmemb), (_size)) ? :
vmefail())
--- findme.c9 Mar 2008 20:24:45 - 1.19
+++ findme.c10 Mar 2008 01:40:21 -
@@ -23,11 +23,10 @@ const char * POPT_findProgramPath(const
if (path == NULL) return NULL;
-start = pathbuf = malloc(strlen(path) + 1);
+start = pathbuf = xstrdup(path);
if (pathbuf == NULL) goto exit;
buf = malloc(strlen(path) + strlen(argv0) + sizeof("/"));
if (buf == NULL) goto exit;
-strcpy(pathbuf, path);
chptr = NULL;
do {
--- popt.c 9 Mar 2008 23:10:05 - 1.120
+++ popt.c 10 Mar 2008 01:40:22 -
@@ -196,10 +196,8 @@ poptContext poptGetContext(const char *
if (getenv("POSIXLY_CORRECT") || getenv("POSIX_ME_HARDER"))
con->flags |= POPT_CONTEXT_POSIXMEHARDER;
-if (name) {
- char * t = malloc(strlen(name) + 1);
- if (t) con->appName = strcpy(t, name);
-}
+if (name)
+ con->appName = xstrdup(name);
invokeCallbacksPRE(con, con->options);
@@ -593,7 +591,7 @@ expandNextArg(/[EMAIL PROTECTED]@*/ poptContext
/[EMAIL PROTECTED] con @*/
{
const char * a = NULL;
-size_t alen;
+size_t alen, pos;
char *t, *te;
size_t tn = strlen(s) + 1;
char c;
@@ -619,10 +617,9 @@ expandNextArg(/[EMAIL PROTECTED]@*/ poptContext
alen = strlen(a);
tn += alen;
- *te = '\0';
+ pos = te - t;
t = realloc(t, tn);
- te = t + strlen(t);
- strncpy(te, a, alen); te += alen;
+ te = stpcpy(t + pos, a);
continue;
/[EMAIL PROTECTED]@*/ /[EMAIL PROTECTED]@*/ break;
default:
@@ -630,8 +627,13 @@ expandNextArg(/[EMAIL PROTECTED]@*/ poptContext
}
*te++ = c;
}
-*te = '\0';
-t = realloc(t, strlen(t) + 1); /* XXX memory leak, hard to plug */
+*te++ = '\0';
+if (t + tn > te) {
+ char *ot = t;
+ t = realloc(t, te - t);
+ if (!t)
+ free(ot);
+}
return t;
}
--- poptconfig.c9 Mar 2008 20:24:45 - 1.37
+++ poptconfig.c10 Mar 2008 01:40:22 -
@@ -224,8 +224,7 @@ int poptReadDefaultConfig(poptContext co
if ((home = getenv("HOME"))) {
fn = malloc(strlen(home) + 20);
if (fn != NULL) {
- strcpy(fn, home);
- strcat(fn, "/.popt");
+ (void)stpcpy(stpcpy(fn, home), "/.popt");
rc = poptReadConfigFile(con, fn);
free(fn);
} else
--- popthelp.c 8 Mar 2008 17:26:30 - 1.85
+++ popthelp.c 10 Mar 2008 01:40:23 -
@@ -237,7 +237,7 @@ singleOptionDefaultValue(size_t lineLeng
if (le == NULL) return NULL; /* XXX can't happen */
*le = '\0';
*le++ = '(';
-strcpy(le, defstr);le += strlen(le);
+le = stpcpy(le, defstr);
*le++ = ':';
*le++ = ' ';
if (opt->arg) { /* XXX programmer error */
@@ -269,7 +269,7 @@ singleOptionDefaultValue(size_t lineLeng
case POPT_ARG_STRING:
{ const char * s = arg.argv[0];
if (s == NULL) {
- s
Re: Question about the xmalloc() et al functions
On Mar 9, 2008, at 7:59 PM, Wayne Davison wrote: On Sun, Mar 09, 2008 at 07:26:31PM -0400, Jeff Johnson wrote: There are some subtleties however with xstrdup. [... mtrace related issues ...] Right, that's why I preserved the HAVE_MCHECK_H version using its own malloc() call (it's unchanged from your version) and just made the non- HAVE_MCHECK_H version add failure checking. All the other functions are the same for both (with exit-on-failure handling). I'd love to use xmalloc everywhere. Alas, there's still lots of dain bread portability issues, the patches to "fix" portability are usually incorrect and limited in scope. See e.g. __secure_getenv and the setresgid/ setresuid "stuff", to name just two. The va_copy() hackery entered popt through the same Newer! Better! Bestest! window, where popt is now undertaking iconv conversions and bind_textdomain_codeset() retrofits that _REALLY_ need to be solved in applications, not in popt. Using xmalloc just opens up a can-of-worms while lusers fuss about non-gcc compiler extension portability. Again, I'd *love* to use xmalloc and xstrdup, I use daily, worksforme. But at some point, popt should just process options, no fuss, no muss, no problems. 73 de Jeff __ POPT Library http://rpm5.org Developer Communication List [email protected]
Re: Question about the xmalloc() et al functions
On Sun, Mar 09, 2008 at 07:26:31PM -0400, Jeff Johnson wrote: > There are some subtleties however with xstrdup. [... mtrace related > issues ...] Right, that's why I preserved the HAVE_MCHECK_H version using its own malloc() call (it's unchanged from your version) and just made the non- HAVE_MCHECK_H version add failure checking. All the other functions are the same for both (with exit-on-failure handling). ..wayne.. __ POPT Library http://rpm5.org Developer Communication List [email protected]
Re: Question about the xmalloc() et al functions
On Mar 9, 2008, at 7:13 PM, Wayne Davison wrote: I'm curious why the xmalloc() function (and its brethren) exits with a fatal error on only some systems and not on all systems? I would think that the code should use the exiting functions in all cases, and just provide differing versions of xstrdup() so that the HAVE_MCHECK_H version does a malloc() (rather than strdup() doing it). xmalloc() et al is mostly a fetish of mine. I personally believe that its silly wasting time checking _ALL_ code paths for malloc failures. 99.999% of the time returning an error rather than aborting is pointless anyways (imho). If I'm not off track, you can apply the attached patch to implement this. There are some subtleties however with xstrdup. If strdup(3), rather than what I use with rpm #define xstrdup(_str) (strcpy((malloc(strlen(_str)+1) ? : vmefail(strlen(_str)+1)), (_str))) is done, then line numbers reported by glibc mtrace(3) will be wrto strdup, not the caller of strdup. All that stops me from using everywhere is the GNU extension that is needed by xstrdup so that the xstrdup macro is mostly side-effect free (I don't care so much that the (_str) macro argument is used several times on the error paths, its used only once on the success paths. And sure a static inline function avoids all the side-effects, but static inline functions were unknown to me last century when the hackery was first devised. valgrind dinna exist, and Rational Purify cost the Very Big $$$ back then too ... 73 de Jeff __ POPT Library http://rpm5.org Developer Communication List [email protected]
