NDT-DEV email list created

[Dominic Hamon <>]

Ok, the patch attached should satisfy those comments. Sorry I missed those ones.

Dominic Hamon | Measurement Lab

http://measurementlab.net

On Fri, Nov 2, 2012 at 1:37 PM, Dominic Hamon <> wrote:

I thought I removed all of those in my last patch. Let me make sure that the patch is cleaned up and I'll send an update when I'm back at my machine.

Dominic Hamon | Measurement Lab

http://measurementlab.net

On Fri, Nov 2, 2012 at 12:47 PM, Aaron Brown <> wrote:

Hey Kavitha/Dominic,

Comment inline.

Cheers,

Aaron

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(
                                        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/

TIP2013, University of Hawaii Mānoa
January 13 - January 17, 2013, Honolulu, HI
http://events.internet2.edu/2013/tip/

Attachment: ndt-sprintf.patch
Description: Binary data