NDT-DEV email list created

[Kavitha Kumar <>]

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...).


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(
                                         buff,
+                                        BUFFSIZE + 1,

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


Thanks!

----- 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
> http://measurementlab.net
> 
> 
> 
> 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
> http://measurementlab.net
> 
> 
> 
> 
> 
> On Wed, Oct 31, 2012 at 3:24 PM, Aaron Brown < 
> 
>  >
> wrote:
> 
> 
> 
> 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
> 
> 
> Cheers,
> Aaron
> 
> 
> 
> 
> 
> On Oct 31, 2012, at 5:30 PM, Dominic Hamon <
> 
>  > wrote:
> 
> 
> 
> 
> 
> Hi
> 
> 
> 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
> http://measurementlab.net
> <ndt-sprintf.patch>
> 
> TIP2013, University of Hawaii Mānoa
> January 13 - January 17, 2013, Honolulu, HI
> http://events.internet2.edu/2013/tip/
> 
> 
>