NDT-DEV email list created

[Kavitha Kumar <>]

All fine. Thank you Dominic!!

----- Original Message -----
> From: "Dominic Hamon" 
> <>
> To: "Aaron Brown" 
> <>,
>  "Kavitha Kumar" 
> <>
> Cc: 
> 
> Sent: Friday, November 2, 2012 6:29:23 PM
> Subject: Re: [ndt-dev] Replacing sprintf with snprintf
> 
> 
> 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/
> 
> 
>