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