[patch] dma-debug: off by one issue

[patch] dma-debug: off by one issue

[Dan Carpenter]

We need to reserve the last character for the NULL terminator.

Signed-off-by: Dan Carpenter <error27@gmail.com>

diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index ba8b670..01e6427 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -570,7 +570,7 @@ static ssize_t filter_write(struct file *file, const char __user *userbuf,
 	 * Now parse out the first token and use it as the name for the
 	 * driver to filter for.
 	 */
-	for (i = 0; i < NAME_MAX_LEN; ++i) {
+	for (i = 0; i < NAME_MAX_LEN - 1; ++i) {
 		current_driver_name[i] = buf[i];
 		if (isspace(buf[i]) || buf[i] = ' ' || buf[i] = 0)
 			break;

Re: [patch] dma-debug: off by one issue

[Joerg Roedel]

On Fri, Apr 02, 2010 at 02:28:43PM +0300, Dan Carpenter wrote:
> We need to reserve the last character for the NULL terminator.

The character is already reserved by the user-to-kernel copy earlier in
the function.

	Joerg

> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> 
> diff --git a/lib/dma-debug.c b/lib/dma-debug.c
> index ba8b670..01e6427 100644
> --- a/lib/dma-debug.c
> +++ b/lib/dma-debug.c
> @@ -570,7 +570,7 @@ static ssize_t filter_write(struct file *file, const char __user *userbuf,
>  	 * Now parse out the first token and use it as the name for the
>  	 * driver to filter for.
>  	 */
> -	for (i = 0; i < NAME_MAX_LEN; ++i) {
> +	for (i = 0; i < NAME_MAX_LEN - 1; ++i) {
>  		current_driver_name[i] = buf[i];
>  		if (isspace(buf[i]) || buf[i] = ' ' || buf[i] = 0)
>  			break;

Re: [patch] dma-debug: off by one issue

[Dan Carpenter]

On Sat, Apr 03, 2010 at 10:47:17PM +0200, Joerg Roedel wrote:
> On Fri, Apr 02, 2010 at 02:28:43PM +0300, Dan Carpenter wrote:
> > We need to reserve the last character for the NULL terminator.
> 
> The character is already reserved by the user-to-kernel copy earlier in
> the function.
> 
> 	Joerg
> 

Yes, but the original code requires more code reading to audit. Let's 
change the commit message to say that it's a clean up patch.

regards,
dan carpenter

> > 
> > Signed-off-by: Dan Carpenter <error27@gmail.com>
> > 
> > diff --git a/lib/dma-debug.c b/lib/dma-debug.c
> > index ba8b670..01e6427 100644
> > --- a/lib/dma-debug.c
> > +++ b/lib/dma-debug.c
> > @@ -570,7 +570,7 @@ static ssize_t filter_write(struct file *file, const char __user *userbuf,
> >  	 * Now parse out the first token and use it as the name for the
> >  	 * driver to filter for.
> >  	 */
> > -	for (i = 0; i < NAME_MAX_LEN; ++i) {
> > +	for (i = 0; i < NAME_MAX_LEN - 1; ++i) {
> >  		current_driver_name[i] = buf[i];
> >  		if (isspace(buf[i]) || buf[i] = ' ' || buf[i] = 0)
> >  			break;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch] dma-debug: off by one issue

[Joerg Roedel]

On Mon, Apr 05, 2010 at 03:53:30PM +0300, Dan Carpenter wrote:
> On Sat, Apr 03, 2010 at 10:47:17PM +0200, Joerg Roedel wrote:

> > The character is already reserved by the user-to-kernel copy earlier in
> > the function.
> 
> Yes, but the original code requires more code reading to audit. Let's 
> change the commit message to say that it's a clean up patch.

In this case its better to remove the

	current_driver_name[i] = 0;

line just after the loop.

	Joerg

Re: [patch] dma-debug: off by one issue

[Joerg Roedel]

On Mon, Apr 05, 2010 at 04:02:15PM +0200, Joerg Roedel wrote:
> On Mon, Apr 05, 2010 at 03:53:30PM +0300, Dan Carpenter wrote:
> > On Sat, Apr 03, 2010 at 10:47:17PM +0200, Joerg Roedel wrote:
> 
> > > The character is already reserved by the user-to-kernel copy earlier in
> > > the function.
> > 
> > Yes, but the original code requires more code reading to audit. Let's 
> > change the commit message to say that it's a clean up patch.
> 
> In this case its better to remove the
> 
> 	current_driver_name[i] = 0;
> 
> line just after the loop.

Ah no, thats nonsense. Mind to resend the patch with a new
commit-message? I'll queue it then tomorrow.

	Joerg

[patch v2] dma-debug: cleanup for loop

[Dan Carpenter]

Earlier in this function we set the last byte of "buf" to NULL so we 
always hit the break statement and "i" is never equal to NAME_MAX_LEN.  
This patch doesn't change how the driver works but it silences a Smatch 
warning and it makes it clearer that we don't write past the end of the 
array.

Signed-off-by: Dan Carpenter <error27@gmail.com>

diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index ba8b670..01e6427 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -570,7 +570,7 @@ static ssize_t filter_write(struct file *file, const char __user *userbuf,
 	 * Now parse out the first token and use it as the name for the
 	 * driver to filter for.
 	 */
-	for (i = 0; i < NAME_MAX_LEN; ++i) {
+	for (i = 0; i < NAME_MAX_LEN - 1; ++i) {
 		current_driver_name[i] = buf[i];
 		if (isspace(buf[i]) || buf[i] = ' ' || buf[i] = 0)
 			break;

Re: [patch v2] dma-debug: cleanup for loop

[Joerg Roedel]

On Tue, Apr 06, 2010 at 07:45:12PM +0300, Dan Carpenter wrote:
> Earlier in this function we set the last byte of "buf" to NULL so we 
> always hit the break statement and "i" is never equal to NAME_MAX_LEN.  
> This patch doesn't change how the driver works but it silences a Smatch 
> warning and it makes it clearer that we don't write past the end of the 
> array.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> 
> diff --git a/lib/dma-debug.c b/lib/dma-debug.c
> index ba8b670..01e6427 100644
> --- a/lib/dma-debug.c
> +++ b/lib/dma-debug.c
> @@ -570,7 +570,7 @@ static ssize_t filter_write(struct file *file, const char __user *userbuf,
>  	 * Now parse out the first token and use it as the name for the
>  	 * driver to filter for.
>  	 */
> -	for (i = 0; i < NAME_MAX_LEN; ++i) {
> +	for (i = 0; i < NAME_MAX_LEN - 1; ++i) {
>  		current_driver_name[i] = buf[i];
>  		if (isspace(buf[i]) || buf[i] = ' ' || buf[i] = 0)
>  			break;

Applied, thanks.