Skip to Content.
Sympa Menu

ndt-dev - Re: [ndt-dev] Replacing sprintf with snprintf

Subject: NDT-DEV email list created

List archive

Re: [ndt-dev] Replacing sprintf with snprintf

Chronological Thread 
  • From: Aaron Brown <>
  • To: Kavitha Kumar <>
  • Cc: Dominic Hamon <>,
  • Subject: Re: [ndt-dev] Replacing sprintf with snprintf
  • Date: Fri, 2 Nov 2012 15:47:09 -0400

Hey Kavitha/Dominic,

Comment inline.


On Nov 2, 2012, at 2:54 PM, Kavitha Kumar <> wrote:

Hi Dominic,

 Here're some suggestions:

1. File src/web100srv.c

--------- Referring to lines near 1420 -----------------
-                strcat(logstr1, logstr2);
-                sprintf(
-                                logstr2,
+                strncat(logstr1, logstr2, sizeof(logstr1));
You may use the "strlcat" method instead.

--------Referring to lines near 2551 ------------------
- sprintf(tmpstr, "go %d %s", t_opts, head_ptr->tests);
+ snprintf(tmpstr, "go %d %s", t_opts, head_ptr->tests);
This snprintf does not have a size associated. This will cause a SIGSEGV and tests will not be completed.
I changed the line above to [ snprintf(tmpstr, TMPSTR_STRLEN, "go %d %s", t_opts, head_ptr->tests); ] and ran some preliminary tests and they seemed to produce expected results.

2.File src/fakewww.c
There are some instances of #define's for buffer size like of HTMLFILE_STRLEN, LINE_STRLEN all being 256. A few of these could be combined into one (for ex, HTMLFILLE_STRLEN is not a str-length…).

Is there any reason to #define any of these? They're all statically defined buffers.

3. File src/analyze.c
--------- Referring to lines near 501 -------------

-                sprintf(tmpstr, "%s/%s", BASEDIR, LOGFILE);
+                snprintf(tmpstr, 256, "%s/%s", BASEDIR, LOGFILE);
You may be able to use: snprintf(tmpstr, sizepf(tmpstr), "%s/%s", BASEDIR, LOGFILE);

If this hardcoded "256" is to ensure that the size is correctly assigned as 256 instead of the "32" from the tpmstr[32] defined outside the "main" function's scope, then, here's what I found: This "tmpstr[32]" seems unused and thus can be deleted, but you may want to double-check.

4. File src/test_mid_srv.c
--------- Referring to lines near 159 -------------
                        send_msg(ctlsockfd, MSG_ERROR, buff, strlen(buff));
@@ -159,7 +159,7 @@ I2Addr         midsrv_addr = NULL; // server address
                log_println(1, "  -- port: %d", options->midsockport);

                // send this port number to client
-                sprintf(buff, "%d", options->midsockport);
+                snprintf(buff, BUFFSIZE + 1, "%d", options->midsockport);
You may be able to use:
snprintf(buff, sizeof(buff), "%d", options->midsockport);

--------- Referring to lines near 250 -------------

@ -250,23 +250,26 @@ I2Addr         midsrv_addr = NULL; // server address
                msgLen = sizeof(buff);
                if (recv_msg(ctlsockfd, &msgType, buff, &msgLen)) { // message reception error
                        log_println(0, "Protocol error!");
-                        sprintf(
+                        snprintf(
+                                        BUFFSIZE + 1,

Instead, suggest:
snprintf(buff, sizeof(buff), "%d", options->midsockport);


----- Original Message -----
From: "Dominic Hamon" <>
To: "Dominic Hamon" <>
Cc: "Aaron Brown" <>,
Sent: Thursday, November 1, 2012 1:07:41 PM
Subject: Re: [ndt-dev] Replacing sprintf with snprintf

A reworked patch attached.

Dominic Hamon | Measurement Lab

On Wed, Oct 31, 2012 at 3:48 PM, Dominic Hamon <
> wrote:

That's possible for those that are defined as array types. However,
for those that are defined within a function as raw pointers, this
would not work as they would have sizeof(char*). I'll rework the
patch to clean the static ones up.

Dominic Hamon | Measurement Lab

On Wed, Oct 31, 2012 at 3:24 PM, Aaron Brown < >

Since all of the buffers are static sized, why not do:

+ snprintf(tmpstr, sizeof(tmpstr), "%s/%s", BASEDIR, LOGFILE);

instead of

+ snprintf(tmpstr, TMPSTR_STRLEN, "%s/%s", BASEDIR, LOGFILE);

Gets rid of the #define's for temporary things


On Oct 31, 2012, at 5:30 PM, Dominic Hamon <
> wrote:


I noticed that there were a few instances of sprintf in the codebase
that weren't taking into account the destination buffer's size. This
seemed a little dangerous to me as buffer runs can lead to invalid
data as well as buffer overflow attacks.

Attached is a patch that replaces every sprintf instance with a call
to sprintf.

Dominic Hamon | Measurement Lab

TIP2013, University of Hawaii Mānoa
January 13 - January 17, 2013, Honolulu, HI

TIP2013, University of Hawaii Mānoa
January 13 - January 17, 2013, Honolulu, HI

Archive powered by MHonArc 2.6.16.

Top of Page