unchecked snprintf: proposed patch

Jim Meyering jim at meyering.net
Mon Mar 21 09:17:46 UTC 2011


Hi Pete,

I noticed that there was a new unchecked use of snprintf.
That can result in silently truncated results.
Then I saw that you'd already done it the right way, using asprintf,
and saw your comment about trouble with an explicit free and GC.

Is this change ok with you?

Can you test it?

Better still, can you outline how I could test it, even if
only in a dry-run mode or something like that?
I'd really like to have a test that's run by "make check"
exercise some of this code.

>From 9584be5a22320649e0fe55fe80d663baf81d6247 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering at redhat.com>
Date: Mon, 21 Mar 2011 10:08:08 +0100
Subject: [PATCH] enable #if-0'd asprintf-using code to avoid snprintf
 truncation risk

* backend.c (fs_rhevm_register): Don't initialize "conf_text"
to NULL or set it to NULL after a failed asprintf call.
Neither is needed with garbage collection.
Remove #if-0, to use asprintf, not snprintf.
Remove explicit free of conf_text.  GC will take care of it.
---
 backend.c |   26 +-------------------------
 1 files changed, 1 insertions(+), 25 deletions(-)

diff --git a/backend.c b/backend.c
index 5b2fd40..c3a91a9 100644
--- a/backend.c
+++ b/backend.c
@@ -1349,7 +1349,7 @@ fs_rhevm_register (my_state *ms, const provider_t *prov, const char *next,
 	char		*nfs_path;
 	char		*nfs_dir;
 	const char	*conf_name = NULL;
-	char		*conf_text = NULL;
+	char		*conf_text;
 	int		 ret	= MHD_HTTP_BAD_REQUEST;
 	int		 rc;
 	const char	*argv[12];
@@ -1439,7 +1439,6 @@ fs_rhevm_register (my_state *ms, const provider_t *prov, const char *next,
 		goto cleanup;
 	}

-#if 0 /* The GC library crashes on free() in this case. */
 	rc = asprintf(&conf_text,
 		    "{\n"
 		    "  \"image\"   : \"%s/%s\",\n"
@@ -1455,31 +1454,9 @@ fs_rhevm_register (my_state *ms, const provider_t *prov, const char *next,
 			nfs_host, nfs_path, nfs_dir
 	    );
 	if (rc < 0) {
-		conf_text = NULL;
 		error (0, 0, "no core");
 		goto cleanup;
 	}
-#else
-	conf_text = malloc(1000);
-	if (!conf_text) {
-		error (0, 0, "no core");
-		goto cleanup;
-	}
-	snprintf(conf_text, 1000,
-		    "{\n"
-		    "  \"image\"   : \"%s/%s\",\n"
-		    "  \"apiurl\"  : \"%s\",\n"
-		    "  \"apiuser\" : \"%s\",\n"
-		    "  \"apipass\" : \"%s\",\n"
-		    "  \"nfshost\" : \"%s\",\n"
-		    "  \"nfspath\" : \"%s\",\n"
-		    "  \"nfsdir\" :  \"%s\"\n"
-		    "}\n",
-			ms->bucket, ms->key,
-			api_url, api_user, api_secret,
-			nfs_host, nfs_path, nfs_dir
-	    );
-#endif
 	conf_name = s3_init_tmpfile(conf_text);
 	if (!conf_name) {
 		error (0, 0, "cannot create a temporary file");
@@ -1568,7 +1545,6 @@ cleanup:
 	if (conf_name)
 		unlink(conf_name);
 	free((char *)conf_name);
-	free(conf_text);

 	(void)meta_set_value(ms->bucket,ms->key,"ami-id",ami_id_buf);
 	return ret;
--
1.7.4.1.514.ga171c


More information about the iwhd-devel mailing list