From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============0313402558119563911==" MIME-Version: 1.0 From: Sergey Senozhatsky Subject: Re: [Powertop] [PATCH v2 6/8] Stop buffer overflow Date: Wed, 15 Oct 2014 21:54:49 +0900 Message-ID: <20141015125449.GB1189@swordfish> In-Reply-To: 1413310159-30577-7-git-send-email-joe.konno@linux.intel.com To: powertop@lists.01.org List-ID: --===============0313402558119563911== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable On (10/14/14 11:09), Joe Konno wrote: I believe Marc MERLIN has reported a crash due to sprint() buffer overrun some (long) time ago. back then my idea (which I never submitted... or did I?...) was to add our own lib.cpp::_sprint() [or whatever the name] and replace all snprintf/sprint at least for buffers with compile time known sizes (aka stack buffers) -- this let us: a) calculate buffer sizeof() within this function and never hit any errors when someone change the buffer size from buf[32] to buf[23] and forget to update all snprint()s b) handle buffer overruns and append too-small buffers with some meaningfull 'watermark' that will give us a hint that that buffer probably needs to be extended. (my choise was '...'). e.g. show 10 device_name_too_lo... instead of 10 device_name_too_lo and we remove a great pile of "numbers, numbers, numbers" from the code - snprintf(name, 20, "%s", all_power[i]->type()); + _sprintf(name, "%s", all_power[i]->type()); c) review can be easier, because we can make a rule to forbid direct sprint()/snprintf() usage (for compile time known buffers). d) I forgot what was my d) point... I still think this is not that bad. any ideas/objections? -ss > Signed-off-by: Joe Konno > --- > src/process/do_process.cpp | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > = > diff --git a/src/process/do_process.cpp b/src/process/do_process.cpp > index dd7d982..2b5a327 100644 > --- a/src/process/do_process.cpp > +++ b/src/process/do_process.cpp > @@ -856,8 +856,8 @@ void process_update_display(void) > = > format_watts(all_power[i]->Witts(), power, 10); > if (!show_power) > - strcpy(power, " "); > - sprintf(name, "%s", all_power[i]->type()); > + strncpy(power, " ", 16); > + snprintf(name, 20, "%s", all_power[i]->type()); > = > align_string(name, 14, 20); > = > @@ -867,9 +867,9 @@ void process_update_display(void) > usage[0] =3D 0; > if (all_power[i]->usage_units()) { > if (all_power[i]->usage() < 1000) > - sprintf(usage, "%5.1f%s", all_power[i]->usage(), all_power[i]->usage= _units()); > + snprintf(usage, 20, "%5.1f%s", all_power[i]->usage(), all_power[i]->= usage_units()); > else > - sprintf(usage, "%5i%s", (int)all_power[i]->usage(), all_power[i]->us= age_units()); > + snprintf(usage, 20, "%5i%s", (int)all_power[i]->usage(), all_power[i= ]->usage_units()); > } > = > align_string(usage, 14, 20); > @@ -878,7 +878,7 @@ void process_update_display(void) > if (!all_power[i]->show_events()) > events[0] =3D 0; > else if (all_power[i]->events() <=3D 0.3) > - sprintf(events, "%5.2f", all_power[i]->events()); > + snprintf(events, 20, "%5.2f", all_power[i]->events()); > = > align_string(events, 12, 20); > wprintw(win, "%s %s %s %s %s\n", power, usage, events, name, pretty_p= rint(all_power[i]->description(), descr, 128)); > -- = > 2.1.2 > = > _______________________________________________ > PowerTop mailing list > PowerTop(a)lists.01.org > https://lists.01.org/mailman/listinfo/powertop >=20 --===============0313402558119563911==--