[dpdk-dev] [PATCH] string: fix potential seg fault on snprintf

[dpdk-dev] [PATCH] string: fix potential seg fault on snprintf

[Pablo de Lara]

From: Pablo de Lara <pablo.de.lara.guarch@intel.com>

Several functions did not check if destination buffer, used
in snprintf was a non-NULL pointer.

Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
---
 examples/cmdline/parse_obj_list.c            |    3 +++
 lib/librte_cmdline/cmdline_parse_etheraddr.c |    3 +++
 lib/librte_cmdline/cmdline_parse_num.c       |    2 +-
 lib/librte_cmdline/cmdline_parse_portlist.c  |    4 ++++
 lib/librte_eal/linuxapp/eal/eal_pci_uio.c    |    3 +++
 5 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/examples/cmdline/parse_obj_list.c b/examples/cmdline/parse_obj_list.c
index 2625ca3..97dfa09 100644
--- a/examples/cmdline/parse_obj_list.c
+++ b/examples/cmdline/parse_obj_list.c
@@ -157,6 +157,9 @@ int complete_get_elt_obj_list(cmdline_parse_token_hdr_t *tk,
 int get_help_obj_list(__attribute__((unused)) cmdline_parse_token_hdr_t *tk,
 		      char *dstbuf, unsigned int size)
 {
+	if (!dstbuf)
+		return -1;
+
 	snprintf(dstbuf, size, "Obj-List");
 	return 0;
 }
diff --git a/lib/librte_cmdline/cmdline_parse_etheraddr.c b/lib/librte_cmdline/cmdline_parse_etheraddr.c
index 5285c40..774b167 100644
--- a/lib/librte_cmdline/cmdline_parse_etheraddr.c
+++ b/lib/librte_cmdline/cmdline_parse_etheraddr.c
@@ -170,6 +170,9 @@ cmdline_get_help_etheraddr(__attribute__((unused)) cmdline_parse_token_hdr_t *tk
 {
 	int ret;
 
+	if (!dstbuf)
+		return -1;
+
 	ret = snprintf(dstbuf, size, "Ethernet address");
 	if (ret < 0)
 		return -1;
diff --git a/lib/librte_cmdline/cmdline_parse_num.c b/lib/librte_cmdline/cmdline_parse_num.c
index 0b9e4d0..26ba5ef 100644
--- a/lib/librte_cmdline/cmdline_parse_num.c
+++ b/lib/librte_cmdline/cmdline_parse_num.c
@@ -349,7 +349,7 @@ cmdline_get_help_num(cmdline_parse_token_hdr_t *tk, char *dstbuf, unsigned int s
 	struct cmdline_token_num_data nd;
 	int ret;
 
-	if (!tk)
+	if (!tk || !dstbuf)
 		return -1;
 
 	memcpy(&nd, &((struct cmdline_token_num *)tk)->num_data, sizeof(nd));
diff --git a/lib/librte_cmdline/cmdline_parse_portlist.c b/lib/librte_cmdline/cmdline_parse_portlist.c
index 7eac05c..8e3521a 100644
--- a/lib/librte_cmdline/cmdline_parse_portlist.c
+++ b/lib/librte_cmdline/cmdline_parse_portlist.c
@@ -163,6 +163,10 @@ cmdline_get_help_portlist(__attribute__((unused)) cmdline_parse_token_hdr_t *tk,
 		char *dstbuf, unsigned int size)
 {
 	int ret;
+
+	if (!dstbuf)
+		return -1;
+
 	ret = snprintf(dstbuf, size, "range of ports as 3,4-6,8-19,20");
 	if (ret < 0)
 		return -1;
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
index 7e62266..ec0ba42 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
@@ -211,6 +211,9 @@ pci_get_uio_dev(struct rte_pci_device *dev, char *dstbuf,
 	DIR *dir;
 	char dirname[PATH_MAX];
 
+	if (!dstbuf)
+		return -1;
+
 	/* depending on kernel version, uio can be located in uio/uioX
 	 * or uio:uioX */
 
-- 
1.7.0.7

Re: [dpdk-dev] [PATCH] string: fix potential seg fault on snprintf

[Olivier MATZ]

Hello Pablo,

On 06/27/2014 01:04 PM, Pablo de Lara wrote:
> From: Pablo de Lara <pablo.de.lara.guarch@intel.com>
>
> Several functions did not check if destination buffer, used
> in snprintf was a non-NULL pointer.

Did you noticed any issue without this patch?

It seems that all the get_help() cmdline functions are never called
with a NULL destination buffer (see in cmdline_parse.c). I think it
is useless to add this test as there is no good reason to give a NULL
argument. It is like testing that the arguments of strcpy() are
non-NULL.

I would say the same for pci_get_uio_dev().

Regards,
Olivier

Re: [dpdk-dev] [PATCH] string: fix potential seg fault on snprintf

[De Lara Guarch, Pablo]

Hi Olivier

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier MATZ
> Sent: Friday, June 27, 2014 12:30 PM
> To: De Lara Guarch, Pablo; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] string: fix potential seg fault on snprintf
> 
> Hello Pablo,
> 
> On 06/27/2014 01:04 PM, Pablo de Lara wrote:
> > From: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> >
> > Several functions did not check if destination buffer, used
> > in snprintf was a non-NULL pointer.
> 
> Did you noticed any issue without this patch?

With last Thomas' patch, cmdline unit test does not pass due to this problem (basically it tests this situation).
After test passed, by fixing this issue in several functions, I looked for other places where this happened.

> It seems that all the get_help() cmdline functions are never called
> with a NULL destination buffer (see in cmdline_parse.c). I think it
> is useless to add this test as there is no good reason to give a NULL
> argument. It is like testing that the arguments of strcpy() are
> non-NULL.
> 
> I would say the same for pci_get_uio_dev().
> 

So, if people prefer to discard this patch, then we should modify the cmdline unit test , 
or we could just include the test only in the places needed for the unit test 
(so, remove it in pci_get_uio_dev, for instance).

Regards,
Pablo

Re: [dpdk-dev] [PATCH] string: fix potential seg fault on snprintf

[Olivier MATZ]

Hi Pablo,

On 06/27/2014 02:13 PM, De Lara Guarch, Pablo wrote:
> With last Thomas' patch, cmdline unit test does not pass due to this problem (basically it tests this situation).
> After test passed, by fixing this issue in several functions, I looked for other places where this happened.

Indeed I missed the unit test, thanks.

I think that testing the NULL case is not required. To me, it is like
testing snprintf(NULL, ...)

Regards,
Olivier

Re: [dpdk-dev] [PATCH] string: fix potential seg fault on snprintf

[Richardson, Bruce]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier MATZ
> Sent: Friday, June 27, 2014 5:34 AM
> To: De Lara Guarch, Pablo; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] string: fix potential seg fault on snprintf
> 
> Hi Pablo,
> 
> On 06/27/2014 02:13 PM, De Lara Guarch, Pablo wrote:
> > With last Thomas' patch, cmdline unit test does not pass due to this problem
> (basically it tests this situation).
> > After test passed, by fixing this issue in several functions, I looked for other
> places where this happened.
> 
> Indeed I missed the unit test, thanks.
> 
> I think that testing the NULL case is not required. To me, it is like
> testing snprintf(NULL, ...)
> 
Famous last words include "that could never happen!" :-)
Since this is not a performance critical piece of code, it does not hurt to leave the Null-check in, and get the additional safety of checking for invalid inputs.

Re: [dpdk-dev] [PATCH] string: fix potential seg fault on snprintf

[Olivier MATZ]

Hello Bruce,

On 06/27/2014 06:36 PM, Richardson, Bruce wrote:
> Famous last words include "that could never happen!" :-)
> Since this is not a performance critical piece of code, it does not hurt to leave the Null-check in, and get the additional safety of checking for invalid inputs.

The "it does not hurt" should not be an argument to keep a patch.
If we follow your reasoning, we should also add tests of
RTE_LOG(..., NULL, ...), rte_eth_dev_*(NULL, ...), ...

In this particular case, giving a NULL argument is meaningless because
the semantic of the function is precisely to write something in the
buffer. Moreover, as I already said, this function is not called by the
user directly.

Regards,
Olivier

Re: [dpdk-dev] [PATCH] string: fix potential seg fault on snprintf

[Richardson, Bruce]

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Monday, June 30, 2014 12:42 AM
> To: Richardson, Bruce; De Lara Guarch, Pablo; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] string: fix potential seg fault on snprintf
> 
> Hello Bruce,
> 
> On 06/27/2014 06:36 PM, Richardson, Bruce wrote:
> > Famous last words include "that could never happen!" :-)
> > Since this is not a performance critical piece of code, it does not hurt to leave
> the Null-check in, and get the additional safety of checking for invalid inputs.
> 
> The "it does not hurt" should not be an argument to keep a patch.
> If we follow your reasoning, we should also add tests of
> RTE_LOG(..., NULL, ...), rte_eth_dev_*(NULL, ...), ...
> 

Actually, I see no issue with adding tests for all those cases. The only time I would agree with not testing all inputs for validity is in a performance critical code path.

> In this particular case, giving a NULL argument is meaningless because
> the semantic of the function is precisely to write something in the
> buffer. Moreover, as I already said, this function is not called by the
> user directly.
> 
> Regards,
> Olivier