patches for DPDK stable branches
 help / color / mirror / Atom feed
From: Aaron Conole <aconole@redhat.com>
To: "Parthasarathy\, JananeeX M" <jananeex.m.parthasarathy@intel.com>
Cc: "'dev\@dpdk.org'" <dev@dpdk.org>, "Pattan\,
	Reshma" <reshma.pattan@intel.com>, "Rao\,
	Nikhil" <nikhil.rao@intel.com>,
	"'stable\@dpdk.org'" <stable@dpdk.org>, "Poornima\,
	PallantlaX" <pallantlax.poornima@intel.com>
Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH] test/eventdev: fix sprintf with snprintf
Date: Tue, 12 Mar 2019 10:44:56 -0400	[thread overview]
Message-ID: <f7tftrsi23b.fsf@dhcp-25.97.bos.redhat.com> (raw)
In-Reply-To: <7AE31235A30B41498D1C31348DC858BD5B54DCD4@IRSMSX103.ger.corp.intel.com> (JananeeX M. Parthasarathy's message of "Tue, 12 Mar 2019 07:41:17 +0000")

"Parthasarathy, JananeeX M" <jananeex.m.parthasarathy@intel.com> writes:

> Hi
>
>>-----Original Message-----
>>From: Parthasarathy, JananeeX M
>>Sent: Tuesday, February 19, 2019 6:33 PM
>>To: Aaron Conole <aconole@redhat.com>; Poornima, PallantlaX
>><pallantlax.poornima@intel.com>
>>Cc: dev@dpdk.org; Pattan, Reshma <reshma.pattan@intel.com>; Rao, Nikhil
>><nikhil.rao@intel.com>; stable@dpdk.org
>>Subject: RE: [dpdk-dev] [PATCH] test/eventdev: fix sprintf with snprintf
>>
>>
>>
>>>-----Original Message-----
>>>From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Aaron Conole
>>>Sent: Saturday, February 09, 2019 2:50 AM
>>>To: Poornima, PallantlaX <pallantlax.poornima@intel.com>
>>>Cc: dev@dpdk.org; Pattan, Reshma <reshma.pattan@intel.com>; Rao, Nikhil
>>><nikhil.rao@intel.com>; stable@dpdk.org
>>>Subject: Re: [dpdk-dev] [PATCH] test/eventdev: fix sprintf with
>>>snprintf
>>>
>>>Pallantla Poornima <pallantlax.poornima@intel.com> writes:
>>>
>>>> sprintf function is not secure as it doesn't check the length of string.
>>>> More secure function snprintf is used.
>>>>
>>>> Fixes: 2a9c83ae3b ("test/eventdev: add multi-ports test")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Pallantla Poornima <pallantlax.poornima@intel.com>
>>>> ---
>>>>  test/test/test_event_eth_rx_adapter.c | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/test/test/test_event_eth_rx_adapter.c
>>>> b/test/test/test_event_eth_rx_adapter.c
>>>> index 1d3be82b5..38f5c039f 100644
>>>> --- a/test/test/test_event_eth_rx_adapter.c
>>>> +++ b/test/test/test_event_eth_rx_adapter.c
>>>> @@ -479,7 +479,8 @@ adapter_multi_eth_add_del(void)
>>>>  	/* add the max port for rx_adapter */
>>>>  	port_index = rte_eth_dev_count_total();
>>>>  	for (; port_index < RTE_MAX_ETHPORTS; port_index += 1) {
>>>> -		sprintf(driver_name, "%s%u", "net_null", drv_id);
>>>> +		snprintf(driver_name, sizeof(driver_name), "%s%u", "net_null",
>>>> +				drv_id);
>>>>  		err = rte_vdev_init(driver_name, NULL);
>>>>  		TEST_ASSERT(err == 0, "Failed driver %s got %d",
>>>>  		driver_name, err);
>>>
>>>You call this a fix, but it's not possible for the value of drv_id to
>>>exceed '32' and the buffer size is plenty accommodating for that.  Did
>>>I miss something?  What is this fixing?
>>
>>It is better practice to use snprintf although in this case buffer will not overflow
>>as size is big enough to accommodate. The changes were done mainly to
>>replace sprintf to snprintf. Probably we can remove "fix" line as it is not issue in
>>this scenario.
>>
>>Thanks
>>M.P.Jananee
>
> Please suggest if we can remove "fix" line.

This is a stylistic change, I don't think it's appropriate to call it a
fix, so I think you can remove the "Fixes" line.

On further reflection, I actually think it will still be wrong.  If the
size buffer is ever changed, what will happen on truncation?  We don't
get an overflow any longer, but we still pass an invalid argument, so I
don't think this 'fix' is really even a fix.  It still has a bug -
albeit not one that immediately triggers SSP exception or stack
overflow.

Makes sense?

> Thanks

  reply	other threads:[~2019-03-12 14:44 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-06 10:43 [dpdk-stable] " Pallantla Poornima
2019-02-08 21:19 ` [dpdk-stable] [dpdk-dev] " Aaron Conole
     [not found]   ` <7AE31235A30B41498D1C31348DC858BD5B534A73@IRSMSX103.ger.corp.intel.com>
2019-03-12  7:41     ` Parthasarathy, JananeeX M
2019-03-12 14:44       ` Aaron Conole [this message]
2019-03-13 11:04         ` Ferruh Yigit
2019-03-13 13:43           ` Aaron Conole
2019-03-13 14:07             ` Ferruh Yigit
2019-03-13 14:35               ` Aaron Conole
2019-03-30 14:15                 ` Jerin Jacob Kollanukkaran
2019-04-01 20:37                   ` Aaron Conole
2019-04-02  1:35                     ` [dpdk-stable] [EXT] " Jerin Jacob Kollanukkaran

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f7tftrsi23b.fsf@dhcp-25.97.bos.redhat.com \
    --to=aconole@redhat.com \
    --cc=dev@dpdk.org \
    --cc=jananeex.m.parthasarathy@intel.com \
    --cc=nikhil.rao@intel.com \
    --cc=pallantlax.poornima@intel.com \
    --cc=reshma.pattan@intel.com \
    --cc=stable@dpdk.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).