[KJ] [PATCH] 3c505.c: replacing strcpy to strncpy

[KJ] [PATCH] 3c505.c: replacing strcpy to strncpy

[Douglas Schilling Landgraf]

Use strncpy instead of strcpy.

Signed-off-by: Douglas Schilling Landgraf <dougsland@gmail.com>

Index: linux-2.6.20.1/drivers/net/3c505.c
=================================--- linux-2.6.20.1.orig/drivers/net/3c505.c
+++ linux-2.6.20.1/drivers/net/3c505.c
@@ -1154,8 +1154,8 @@ static struct net_device_stats *elp_get_
 static void netdev_get_drvinfo(struct net_device *dev,
 			       struct ethtool_drvinfo *info)
 {
-	strcpy(info->driver, DRV_NAME);
-	strcpy(info->version, DRV_VERSION);
+	strncpy(info->driver, DRV_NAME, sizeof(info->driver)-1);
+	strncpy(info->version, DRV_VERSION, sizeof(info->driver)-1);
 	sprintf(info->bus_info, "ISA 0x%lx", dev->base_addr);
 }
 
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] 3c505.c: replacing strcpy to strncpy

[Milind Choudhary]

On 2/25/07, Douglas Schilling Landgraf <dougsland@gmail.com> wrote:
> Use strncpy instead of strcpy.
>
> Signed-off-by: Douglas Schilling Landgraf <dougsland@gmail.com>
>
> Index: linux-2.6.20.1/drivers/net/3c505.c
> =================================> --- linux-2.6.20.1.orig/drivers/net/3c505.c
> +++ linux-2.6.20.1/drivers/net/3c505.c
> @@ -1154,8 +1154,8 @@ static struct net_device_stats *elp_get_
>  static void netdev_get_drvinfo(struct net_device *dev,
>                                struct ethtool_drvinfo *info)
>  {
> -       strcpy(info->driver, DRV_NAME);
> -       strcpy(info->version, DRV_VERSION);
> +       strncpy(info->driver, DRV_NAME, sizeof(info->driver)-1);
> +       strncpy(info->version, DRV_VERSION, sizeof(info->driver)-1);

          ^^^^^^^^^^
shouldn't this be info-version instead

-- 
Milind Arun Choudhary
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

[KJ] [PATCH] 3c505.c: replacing strcpy to strncpy

[Douglas Schilling Landgraf]

Use strncpy instead of strcpy.

Signed-off-by: Douglas Schilling Landgraf <dougsland@gmail.com>

Index: linux-2.6.20.1/drivers/net/3c505.c
=================================--- linux-2.6.20.1.orig/drivers/net/3c505.c
+++ linux-2.6.20.1/drivers/net/3c505.c
@@ -1154,8 +1154,8 @@ static struct net_device_stats *elp_get_
 static void netdev_get_drvinfo(struct net_device *dev,
 			       struct ethtool_drvinfo *info)
 {
-	strcpy(info->driver, DRV_NAME);
-	strcpy(info->version, DRV_VERSION);
+	strncpy(info->driver, DRV_NAME, sizeof(info->driver)-1);
+	strncpy(info->version, DRV_VERSION, sizeof(info->version)-1);
 	sprintf(info->bus_info, "ISA 0x%lx", dev->base_addr);
 }
 

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] 3c505.c: replacing strcpy to strncpy

[burns.ethan]

On Mon, Feb 26, 2007 at 10:10:36PM -0300, Douglas Schilling Landgraf wrote:
> Use strncpy instead of strcpy.

Is there ever a reason to use strcpy() over strncpy()?  If not, there's
probably another todo item here:

~/src/linux-2.6 $ grep -r strcpy . | wc -l
2592


		--Ethan
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] 3c505.c: replacing strcpy to strncpy

[Standard Azi]

char buf[150];

strcpy(buf, "LALA");

that's one reason, for example

On 2/27/07, burns.ethan@gmail.com <burns.ethan@gmail.com> wrote:
> On Mon, Feb 26, 2007 at 10:10:36PM -0300, Douglas Schilling Landgraf wrote:
> > Use strncpy instead of strcpy.
>
> Is there ever a reason to use strcpy() over strncpy()?  If not, there's
> probably another todo item here:
>
> ~/src/linux-2.6 $ grep -r strcpy . | wc -l
> 2592
>
>
>                 --Ethan
> _______________________________________________
> Kernel-janitors mailing list
> Kernel-janitors@lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/kernel-janitors
>
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] 3c505.c: replacing strcpy to strncpy

[Bernhard R. Link]

* burns.ethan@gmail.com <burns.ethan@gmail.com> [070227 01:33]:
> > Use strncpy instead of strcpy.
> 
> Is there ever a reason to use strcpy() over strncpy()?  If not, there's
> probably another todo item here:
> 
> ~/src/linux-2.6 $ grep -r strcpy . | wc -l
> 2592

Replacing a strcpy with a strncpy usually makes code more buggy than
it was before, as strncpy does not generate 0-terminated strings when
the input is too long. (Unless the kernel names something strncpy
which is no strncpy, I've no kernel source unpackages around, but I
doubt that). Even if the output is 0-terminated, simply truncating it
can be a security problem in some cases. (And a problem in most cases)

strcpy is good if you are sure something fits in the destination. If
not, you should test before and if it fits not, give an appropiate
response, either truncate where it is save, or simply fail. (And when
you tested its length and still have that around, you need no strcpy or
strncpy but a memcpy can be used, making it even faster on some arches).

So I'd rather suggest looking for strncpy and checking those. I don't
know what the state of strncpy in the kernel is, but in an avarage
project strncpy usage typically means a bug.

Hochachtungsvoll,
	Bernhard R. Link
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] 3c505.c: replacing strcpy to strncpy

[burns.ethan]

On Tue, Feb 27, 2007 at 10:03:25AM +0100, Standard Azi wrote:
> char buf[150];
>
> strcpy(buf, "LALA");
>
> that's one reason, for example

char buf[150] = "LALA";

and if you really need to do it outside of initialization, does the strncpy()
really hurt it?  What if the size of `buf' gets changed, then we would have to
go check for all strcpy()s and make sure that they are still valid, where
strncpy would do it by default.

	--Ethan
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] 3c505.c: replacing strcpy to strncpy

[burns.ethan]

On Tue, Feb 27, 2007 at 10:07:43AM +0100, Bernhard R. Link wrote:
> Replacing a strcpy with a strncpy usually makes code more buggy than
> it was before, as strncpy does not generate 0-terminated strings when
> the input is too long. (Unless the kernel names something strncpy
> which is no strncpy, I've no kernel source unpackages around, but I
> doubt that). Even if the output is 0-terminated, simply truncating it
> can be a security problem in some cases. (And a problem in most cases)
>
> strcpy is good if you are sure something fits in the destination. If
> not, you should test before and if it fits not, give an appropiate
> response, either truncate where it is save, or simply fail. (And when
> you tested its length and still have that around, you need no strcpy or
> strncpy but a memcpy can be used, making it even faster on some arches).
>
> So I'd rather suggest looking for strncpy and checking those. I don't
> know what the state of strncpy in the kernel is, but in an avarage
> project strncpy usage typically means a bug.

Looks like strlcpy() would solve both these problems.  It guarentees that the
destination string is null terminated and it returns the strlen() of the
source string:

	ssize = strlcpy(dest, src, dsize);
	if (ssize >= dsize - 1)
		return ERROR;

Here we have dest guarenteed to be terminated, and if the source string is
greater than the dest - 1 (for the '\0'), we can bail out.

		--Ethan
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] 3c505.c: replacing strcpy to strncpy

[Bernhard R. Link]

* burns.ethan@gmail.com <burns.ethan@gmail.com> [070227 17:41]:
> Looks like strlcpy() would solve both these problems.

While there may be cases where it is useful, it normaly solves
a problem that is not there.

        ssize = strlen(src) + 1;
        if( ssize > dsize )
                return ERROR;
        memcpy(dest, src, ssize);

is the same like

> 	ssize = strlcpy(dest, src, dsize);
> 	if (ssize >= dsize - 1)
> 		return ERROR;
> 
> Here we have dest guarenteed to be terminated, and if the source string is
> greater than the dest - 1 (for the '\0'), we can bail out.

except you test for greater or equal instead of greater ;->

Hochachtungsvoll,
	Bernhard R. Link
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] 3c505.c: replacing strcpy to strncpy

[burns.ethan]

On Tue, Feb 27, 2007 at 06:31:18PM +0100, Bernhard R. Link wrote:
> * burns.ethan@gmail.com <burns.ethan@gmail.com> [070227 17:41]:
> > Looks like strlcpy() would solve both these problems.
>
> While there may be cases where it is useful, it normally solves
> a problem that is not there.
>
>         ssize = strlen(src) + 1;
>         if( ssize > dsize )
>                 return ERROR;
>         memcpy(dest, src, ssize);
>
> is the same like
>
> > 	ssize = strlcpy(dest, src, dsize);
> > 	if (ssize >= dsize - 1)
> > 		return ERROR;
> >
> > Here we have dest guaranteed to be terminated, and if the source string is
> > greater than the dest - 1 (for the '\0'), we can bail out.

Sure, however your way allows for the dest buff length test to be left off
(forgotten).  If the convention was to use strlcpy() the test is forced by
requiring the length argument to be passed.  strlcpy() uses memcpy()
under-the-hood anyway so there's no real efficiency differences here, AFAIK.

Also, the strlcpy() version is 1 line shorter (which is not *terribly*
important) and arguably more readable (not a very strong argument).

Finally if truncation is not a problem, strlcpy() handles this case just
as fine as anything and is probably preferable to a memcpy() with a length
test.  So might as well use strlcpy() in both places for consistency.


                --Ethan
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] 3c505.c: replacing strcpy to strncpy

[Douglas Landgraf]

Hi guys,

    I agree with Ethan, we can use strlcpy() instead of
strcpy()/strncpy()/memcpy() in this case.

Nowadays, we already have two modules using this function:
[douglas@localhost net]$ grep strlcpy *.c
acenic.c:       strlcpy(info->driver, "acenic", sizeof(info->driver));
acenic.c:               strlcpy(info->bus_info, pci_name(ap->pdev),
acenic.mod.c:   { 0x73e20c1c, "strlcpy" },
hp100.c:        strlcpy(lp->id, eid, HP100_SIG_LEN);


Note this part of strlcpy article from http://lwn.net:

"Linus agreed that following OpenBSD's lead was the right way forward,
and strlcpy() is in his BitKeeper repository, waiting for 2.5.71.
There has also been a flurry of activity to convert kernel code over
to the new function. By the time 2.6.0 comes out, strncpy() may no
longer have a place in the Linux kernel."

Source: http://lwn.net/Articles/33812/

Sincerely,
Douglas Schilling Landgraf

On 2/27/07, burns.ethan@gmail.com <burns.ethan@gmail.com> wrote:
> On Tue, Feb 27, 2007 at 06:31:18PM +0100, Bernhard R. Link wrote:
> > * burns.ethan@gmail.com <burns.ethan@gmail.com> [070227 17:41]:
> > > Looks like strlcpy() would solve both these problems.
> >
> > While there may be cases where it is useful, it normally solves
> > a problem that is not there.
> >
> >         ssize = strlen(src) + 1;
> >         if( ssize > dsize )
> >                 return ERROR;
> >         memcpy(dest, src, ssize);
> >
> > is the same like
> >
> > >     ssize = strlcpy(dest, src, dsize);
> > >     if (ssize >= dsize - 1)
> > >             return ERROR;
> > >
> > > Here we have dest guaranteed to be terminated, and if the source string is
> > > greater than the dest - 1 (for the '\0'), we can bail out.
>
> Sure, however your way allows for the dest buff length test to be left off
> (forgotten).  If the convention was to use strlcpy() the test is forced by
> requiring the length argument to be passed.  strlcpy() uses memcpy()
> under-the-hood anyway so there's no real efficiency differences here, AFAIK.
>
> Also, the strlcpy() version is 1 line shorter (which is not *terribly*
> important) and arguably more readable (not a very strong argument).
>
> Finally if truncation is not a problem, strlcpy() handles this case just
> as fine as anything and is probably preferable to a memcpy() with a length
> test.  So might as well use strlcpy() in both places for consistency.
>
>
>                 --Ethan
> _______________________________________________
> Kernel-janitors mailing list
> Kernel-janitors@lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/kernel-janitors
>
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] 3c505.c: replacing strcpy to strncpy

[Ira Snyder]

[-- Attachment #1.1: Type: text/plain, Size: 792 bytes --]

On Tue, 27 Feb 2007 11:12:00 -0500
burns.ethan@gmail.com wrote:

> On Tue, Feb 27, 2007 at 10:03:25AM +0100, Standard Azi wrote:
> > char buf[150];
> >
> > strcpy(buf, "LALA");
> >
> > that's one reason, for example
>
> char buf[150] = "LALA";
>
> and if you really need to do it outside of initialization, does the strncpy()
> really hurt it?  What if the size of `buf' gets changed, then we would have to
> go check for all strcpy()s and make sure that they are still valid, where
> strncpy would do it by default.
>

Also, what about strlcpy()? It's a drop-in replacement for strncpy(), _ALWAYS_ null terminates the destination string (strncpy() doesn't), and doesn't have to pad out the destination string strncpy().

I pretty much always find it better than strcpy() and strncpy().

Ira

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors