[dpdk-dev,5/5] cfgfile: increase local buffer size for max name and value

  From: Joseph Richard <joseph.richard@windriver.com>

When parsing a ini file with a "key = value" line that has both "key" and
"value" sized to the maximum allowed length causes a parsing failure.  The
internal "buffer" variable should be sized at least as large as the maximum
for both fields.  This commit updates the local array to be sized to hold
the max name, max value, " = ", and the nul terminator.

Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
---
 lib/librte_cfgfile/rte_cfgfile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

> On Mar 2, 2017, at 1:29 PM, Allain Legacy <allain.legacy@windriver.com> wrote:
> 
> From: Joseph Richard <joseph.richard@windriver.com>
> 
> When parsing a ini file with a "key = value" line that has both "key" and
> "value" sized to the maximum allowed length causes a parsing failure.  The
> internal "buffer" variable should be sized at least as large as the maximum
> for both fields.  This commit updates the local array to be sized to hold
> the max name, max value, " = ", and the nul terminator.
> 
> Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
> ---
> lib/librte_cfgfile/rte_cfgfile.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
> index 28956ea..107d637 100644
> --- a/lib/librte_cfgfile/rte_cfgfile.c
> +++ b/lib/librte_cfgfile/rte_cfgfile.c
> @@ -92,7 +92,7 @@ struct rte_cfgfile *
> 	int allocated_entries = 0;
> 	int curr_section = -1;
> 	int curr_entry = -1;
> -	char buffer[256] = {0};
> +	char buffer[CFG_NAME_LEN + CFG_VALUE_LEN + 4] = {0};

Would this change still cause a failure and memory over write if the user decides to have very large string. Does the code check the lengths to make sure they are valid and return error?

If the code is testing the size and make sure a memory over write does not happen, then I am OK with acking this patch. 

> 	int lineno = 0;
> 	size_t size;
> 	struct rte_cfgfile *cfg = NULL;
> -- 
> 1.8.3.1
> 

Regards,
Keith
  

> -----Original Message-----
> From: Wiles, Keith [mailto:keith.wiles@intel.com]
> Sent: Thursday, March 09, 2017 8:46 AM
> Would this change still cause a failure and memory over write if the user
> decides to have very large string. Does the code check the lengths to make
> sure they are valid and return error?
> 

The fgets() is bounded by the size of the buffer and the subsequent validation will raise an error if no newline was detected within the buffer therefore an overly long line will result in a failure.  I have added a test case in the v2 patchset in which I have added a unit test framework for this library.

	while (fgets(buffer, sizeof(buffer), f) != NULL) {
		char *pos = NULL;
		size_t len = strnlen(buffer, sizeof(buffer));
		lineno++;
		if ((len >= sizeof(buffer) - 1) && (buffer[len-1] != '\n')) {
			printf("Error line %d - no \\n found on string. "
					"Check if line too long\n", lineno);
			goto error1;
		}

Does that satisfy your concern and qualify for you Ack?
  

> On Mar 9, 2017, at 9:16 AM, Legacy, Allain <Allain.Legacy@windriver.com> wrote:
> 
>> -----Original Message-----
>> From: Wiles, Keith [mailto:keith.wiles@intel.com]
>> Sent: Thursday, March 09, 2017 8:46 AM
>> Would this change still cause a failure and memory over write if the user
>> decides to have very large string. Does the code check the lengths to make
>> sure they are valid and return error?
>> 
> 
> The fgets() is bounded by the size of the buffer and the subsequent validation will raise an error if no newline was detected within the buffer therefore an overly long line will result in a failure.  I have added a test case in the v2 patchset in which I have added a unit test framework for this library.
> 
> 	while (fgets(buffer, sizeof(buffer), f) != NULL) {
> 		char *pos = NULL;
> 		size_t len = strnlen(buffer, sizeof(buffer));
> 		lineno++;
> 		if ((len >= sizeof(buffer) - 1) && (buffer[len-1] != '\n')) {
> 			printf("Error line %d - no \\n found on string. "
> 					"Check if line too long\n", lineno);
> 			goto error1;
> 		}
> 
> Does that satisfy your concern and qualify for you Ack?

Acked-by: Keith Wiles <keith.wiles@intel.com>

Regards,
Keith