* [patch] net/tipc: sprintf/strcpy conversion
@ 2006-11-01 14:06 ` Florian Westphal
0 siblings, 0 replies; 17+ messages in thread
From: Florian Westphal @ 2006-11-01 14:06 UTC (permalink / raw)
To: kernel-janitors; +Cc: tipc-discussion, netdev
From: Florian Westphal <fw@strlen.de>
convert sprintf(a,b) to strcpy(a,b). Make tipc_bclink_name[] const.
Signed-off-by: Florian Westphal <fw@strlen.de>
---
compile tested; diffed against davem/net-2.6.
--- a/net/tipc/bcast.c
+++ b/net/tipc/bcast.c
@@ -119,7 +119,7 @@ static struct bclink *bclink = NULL;
static struct link *bcl = NULL;
static DEFINE_SPINLOCK(bc_lock);
-char tipc_bclink_name[] = "multicast-link";
+const char tipc_bclink_name[] = "multicast-link";
static u32 buf_seqno(struct sk_buff *buf)
@@ -790,7 +790,7 @@ int tipc_bclink_init(void)
INIT_LIST_HEAD(&bcbearer->bearer.cong_links);
bcbearer->bearer.media = &bcbearer->media;
bcbearer->media.send_msg = tipc_bcbearer_send;
- sprintf(bcbearer->media.name, "tipc-multicast");
+ strcpy(bcbearer->media.name, "tipc-multicast");
bcl = &bclink->link;
memset(bclink, 0, sizeof(struct bclink));
@@ -802,7 +802,7 @@ int tipc_bclink_init(void)
tipc_link_set_queue_limits(bcl, BCLINK_WIN_DEFAULT);
bcl->b_ptr = &bcbearer->bearer;
bcl->state = WORKING_WORKING;
- sprintf(bcl->name, tipc_bclink_name);
+ strcpy(bcl->name, tipc_bclink_name);
if (BCLINK_LOG_BUF_SIZE) {
char *pb = kmalloc(BCLINK_LOG_BUF_SIZE, GFP_ATOMIC);
diff --git a/net/tipc/bcast.h b/net/tipc/bcast.h
--- a/net/tipc/bcast.h
+++ b/net/tipc/bcast.h
@@ -70,7 +70,7 @@ struct port_list {
struct node;
-extern char tipc_bclink_name[];
+extern const char tipc_bclink_name[];
/**
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -667,7 +667,7 @@ struct sk_buff *tipc_node_get_links(cons
link_info.dest = tipc_own_addr & 0xfffff00;
link_info.dest = htonl(link_info.dest);
link_info.up = htonl(1);
- sprintf(link_info.str, tipc_bclink_name);
+ strcpy(link_info.str, tipc_bclink_name);
tipc_cfg_append_tlv(buf, TIPC_TLV_LINK_INFO, &link_info, sizeof(link_info));
/* Add TLVs for any other links in scope */
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [KJ] [patch] net/tipc: sprintf/strcpy conversion
2006-11-01 14:06 ` Florian Westphal
@ 2006-11-01 21:38 ` walter harms
-1 siblings, 0 replies; 17+ messages in thread
From: walter harms @ 2006-11-01 21:38 UTC (permalink / raw)
To: kernel-janitors, tipc-discussion, netdev
hi Florian,
These line
+ strcpy(bcbearer->media.name, "tipc-multicast");
i gues that means tipc_bclink_name ?
an even more secure version could be like this:
strncpy(bcbearer->media.name,sizeof(bcbearer->media.name),tipc_bclink_name);
(in case someone ever changes the size of cbearer->media.name or tipc_bclink_name and the hope
that wchat_t will never reach the kernel)
re,
wh
Florian Westphal wrote:
> From: Florian Westphal <fw@strlen.de>
>
> convert sprintf(a,b) to strcpy(a,b). Make tipc_bclink_name[] const.
>
> Signed-off-by: Florian Westphal <fw@strlen.de>
>
> ---
>
> compile tested; diffed against davem/net-2.6.
>
> --- a/net/tipc/bcast.c
> +++ b/net/tipc/bcast.c
> @@ -119,7 +119,7 @@ static struct bclink *bclink = NULL;
> static struct link *bcl = NULL;
> static DEFINE_SPINLOCK(bc_lock);
>
> -char tipc_bclink_name[] = "multicast-link";
> +const char tipc_bclink_name[] = "multicast-link";
>
>
> static u32 buf_seqno(struct sk_buff *buf)
> @@ -790,7 +790,7 @@ int tipc_bclink_init(void)
> INIT_LIST_HEAD(&bcbearer->bearer.cong_links);
> bcbearer->bearer.media = &bcbearer->media;
> bcbearer->media.send_msg = tipc_bcbearer_send;
> - sprintf(bcbearer->media.name, "tipc-multicast");
> + strcpy(bcbearer->media.name, "tipc-multicast");
>
> bcl = &bclink->link;
> memset(bclink, 0, sizeof(struct bclink));
> @@ -802,7 +802,7 @@ int tipc_bclink_init(void)
> tipc_link_set_queue_limits(bcl, BCLINK_WIN_DEFAULT);
> bcl->b_ptr = &bcbearer->bearer;
> bcl->state = WORKING_WORKING;
> - sprintf(bcl->name, tipc_bclink_name);
> + strcpy(bcl->name, tipc_bclink_name);
>
> if (BCLINK_LOG_BUF_SIZE) {
> char *pb = kmalloc(BCLINK_LOG_BUF_SIZE, GFP_ATOMIC);
> diff --git a/net/tipc/bcast.h b/net/tipc/bcast.h
> --- a/net/tipc/bcast.h
> +++ b/net/tipc/bcast.h
> @@ -70,7 +70,7 @@ struct port_list {
>
> struct node;
>
> -extern char tipc_bclink_name[];
> +extern const char tipc_bclink_name[];
>
>
> /**
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -667,7 +667,7 @@ struct sk_buff *tipc_node_get_links(cons
> link_info.dest = tipc_own_addr & 0xfffff00;
> link_info.dest = htonl(link_info.dest);
> link_info.up = htonl(1);
> - sprintf(link_info.str, tipc_bclink_name);
> + strcpy(link_info.str, tipc_bclink_name);
> tipc_cfg_append_tlv(buf, TIPC_TLV_LINK_INFO, &link_info, sizeof(link_info));
>
> /* Add TLVs for any other links in scope */
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [KJ] [patch] net/tipc: sprintf/strcpy conversion
@ 2006-11-01 21:38 ` walter harms
0 siblings, 0 replies; 17+ messages in thread
From: walter harms @ 2006-11-01 21:38 UTC (permalink / raw)
To: kernel-janitors, tipc-discussion, netdev
hi Florian,
These line
+ strcpy(bcbearer->media.name, "tipc-multicast");
i gues that means tipc_bclink_name ?
an even more secure version could be like this:
strncpy(bcbearer->media.name,sizeof(bcbearer->media.name),tipc_bclink_name);
(in case someone ever changes the size of cbearer->media.name or tipc_bclink_name and the hope
that wchat_t will never reach the kernel)
re,
wh
Florian Westphal wrote:
> From: Florian Westphal <fw@strlen.de>
>
> convert sprintf(a,b) to strcpy(a,b). Make tipc_bclink_name[] const.
>
> Signed-off-by: Florian Westphal <fw@strlen.de>
>
> ---
>
> compile tested; diffed against davem/net-2.6.
>
> --- a/net/tipc/bcast.c
> +++ b/net/tipc/bcast.c
> @@ -119,7 +119,7 @@ static struct bclink *bclink = NULL;
> static struct link *bcl = NULL;
> static DEFINE_SPINLOCK(bc_lock);
>
> -char tipc_bclink_name[] = "multicast-link";
> +const char tipc_bclink_name[] = "multicast-link";
>
>
> static u32 buf_seqno(struct sk_buff *buf)
> @@ -790,7 +790,7 @@ int tipc_bclink_init(void)
> INIT_LIST_HEAD(&bcbearer->bearer.cong_links);
> bcbearer->bearer.media = &bcbearer->media;
> bcbearer->media.send_msg = tipc_bcbearer_send;
> - sprintf(bcbearer->media.name, "tipc-multicast");
> + strcpy(bcbearer->media.name, "tipc-multicast");
>
> bcl = &bclink->link;
> memset(bclink, 0, sizeof(struct bclink));
> @@ -802,7 +802,7 @@ int tipc_bclink_init(void)
> tipc_link_set_queue_limits(bcl, BCLINK_WIN_DEFAULT);
> bcl->b_ptr = &bcbearer->bearer;
> bcl->state = WORKING_WORKING;
> - sprintf(bcl->name, tipc_bclink_name);
> + strcpy(bcl->name, tipc_bclink_name);
>
> if (BCLINK_LOG_BUF_SIZE) {
> char *pb = kmalloc(BCLINK_LOG_BUF_SIZE, GFP_ATOMIC);
> diff --git a/net/tipc/bcast.h b/net/tipc/bcast.h
> --- a/net/tipc/bcast.h
> +++ b/net/tipc/bcast.h
> @@ -70,7 +70,7 @@ struct port_list {
>
> struct node;
>
> -extern char tipc_bclink_name[];
> +extern const char tipc_bclink_name[];
>
>
> /**
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -667,7 +667,7 @@ struct sk_buff *tipc_node_get_links(cons
> link_info.dest = tipc_own_addr & 0xfffff00;
> link_info.dest = htonl(link_info.dest);
> link_info.up = htonl(1);
> - sprintf(link_info.str, tipc_bclink_name);
> + strcpy(link_info.str, tipc_bclink_name);
> tipc_cfg_append_tlv(buf, TIPC_TLV_LINK_INFO, &link_info, sizeof(link_info));
>
> /* Add TLVs for any other links in scope */
> _______________________________________________
> Kernel-janitors mailing list
> Kernel-janitors@lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/kernel-janitors
>
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [KJ] [patch] net/tipc: sprintf/strcpy conversion
2006-11-01 21:38 ` walter harms
@ 2006-11-01 23:19 ` David Miller
-1 siblings, 0 replies; 17+ messages in thread
From: David Miller @ 2006-11-01 23:19 UTC (permalink / raw)
To: wharms; +Cc: kernel-janitors, tipc-discussion, netdev
From: walter harms <wharms@bfs.de>
Date: Wed, 01 Nov 2006 22:38:26 +0100
> These line
> + strcpy(bcbearer->media.name, "tipc-multicast");
>
> i gues that means tipc_bclink_name ?
Why? The original code used "tipc-multicast" which is a
different string than tipc_bclink_name which is "multicast-link".
> > - sprintf(bcbearer->media.name, "tipc-multicast");
> > + strcpy(bcbearer->media.name, "tipc-multicast");
If you are arguing that it should be changed, that's a different
changeset to discuss.
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [KJ] [patch] net/tipc: sprintf/strcpy conversion
@ 2006-11-01 23:19 ` David Miller
0 siblings, 0 replies; 17+ messages in thread
From: David Miller @ 2006-11-01 23:19 UTC (permalink / raw)
To: wharms; +Cc: kernel-janitors, tipc-discussion, netdev
From: walter harms <wharms@bfs.de>
Date: Wed, 01 Nov 2006 22:38:26 +0100
> These line
> + strcpy(bcbearer->media.name, "tipc-multicast");
>
> i gues that means tipc_bclink_name ?
Why? The original code used "tipc-multicast" which is a
different string than tipc_bclink_name which is "multicast-link".
> > - sprintf(bcbearer->media.name, "tipc-multicast");
> > + strcpy(bcbearer->media.name, "tipc-multicast");
If you are arguing that it should be changed, that's a different
changeset to discuss.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [KJ] [patch] net/tipc: sprintf/strcpy conversion
2006-11-01 23:19 ` David Miller
@ 2006-11-02 7:45 ` walter harms
-1 siblings, 0 replies; 17+ messages in thread
From: walter harms @ 2006-11-02 7:45 UTC (permalink / raw)
To: David Miller; +Cc: kernel-janitors, tipc-discussion, netdev
David Miller wrote:
> From: walter harms <wharms@bfs.de>
> Date: Wed, 01 Nov 2006 22:38:26 +0100
>
>> These line
>> + strcpy(bcbearer->media.name, "tipc-multicast");
>>
>> i gues that means tipc_bclink_name ?
mea culpa, i should not write mail when tired.
>
> Why? The original code used "tipc-multicast" which is a
> different string than tipc_bclink_name which is "multicast-link".
>
>>> - sprintf(bcbearer->media.name, "tipc-multicast");
>>> + strcpy(bcbearer->media.name, "tipc-multicast");
>
> If you are arguing that it should be changed, that's a different
> changeset to discuss.
>
>
>
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [KJ] [patch] net/tipc: sprintf/strcpy conversion
@ 2006-11-02 7:45 ` walter harms
0 siblings, 0 replies; 17+ messages in thread
From: walter harms @ 2006-11-02 7:45 UTC (permalink / raw)
To: David Miller; +Cc: kernel-janitors, tipc-discussion, netdev
David Miller wrote:
> From: walter harms <wharms@bfs.de>
> Date: Wed, 01 Nov 2006 22:38:26 +0100
>
>> These line
>> + strcpy(bcbearer->media.name, "tipc-multicast");
>>
>> i gues that means tipc_bclink_name ?
mea culpa, i should not write mail when tired.
>
> Why? The original code used "tipc-multicast" which is a
> different string than tipc_bclink_name which is "multicast-link".
>
>>> - sprintf(bcbearer->media.name, "tipc-multicast");
>>> + strcpy(bcbearer->media.name, "tipc-multicast");
>
> If you are arguing that it should be changed, that's a different
> changeset to discuss.
>
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [KJ] [tipc-discussion] [patch] net/tipc: sprintf/strcpy
2006-11-01 21:38 ` walter harms
@ 2006-11-02 6:26 ` Florian Westphal
-1 siblings, 0 replies; 17+ messages in thread
From: Florian Westphal @ 2006-11-02 6:26 UTC (permalink / raw)
To: tipc-discussion, kernel-janitors, netdev
walter harms <wharms@bfs.de> wrote:
> These line
>+ strcpy(bcbearer->media.name, "tipc-multicast");
> i gues that means tipc_bclink_name ?
The idea was to change how things are done, not _what_ is being done.
> an even more secure version could be like this:
>
> strncpy(bcbearer->media.name,sizeof(bcbearer->media.name),tipc_bclink_name);
Ugh, please, no. The size of src is known in all cases; there is
absoluty no point in using str(n|l)cpy here.
> (in case someone ever changes the size of cbearer->media.name or tipc_bclink_name and the hope
> that wchat_t will never reach the kernel)
In this case 'someone' should be really hurt, don't you think?
Florian
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [tipc-discussion] [KJ] [patch] net/tipc: sprintf/strcpy conversion
@ 2006-11-02 6:26 ` Florian Westphal
0 siblings, 0 replies; 17+ messages in thread
From: Florian Westphal @ 2006-11-02 6:26 UTC (permalink / raw)
To: tipc-discussion, kernel-janitors, netdev
walter harms <wharms@bfs.de> wrote:
> These line
>+ strcpy(bcbearer->media.name, "tipc-multicast");
> i gues that means tipc_bclink_name ?
The idea was to change how things are done, not _what_ is being done.
> an even more secure version could be like this:
>
> strncpy(bcbearer->media.name,sizeof(bcbearer->media.name),tipc_bclink_name);
Ugh, please, no. The size of src is known in all cases; there is
absoluty no point in using str(n|l)cpy here.
> (in case someone ever changes the size of cbearer->media.name or tipc_bclink_name and the hope
> that wchat_t will never reach the kernel)
In this case 'someone' should be really hurt, don't you think?
Florian
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [KJ] [tipc-discussion] [patch] net/tipc:
2006-11-02 6:26 ` [tipc-discussion] [KJ] [patch] net/tipc: sprintf/strcpy conversion Florian Westphal
@ 2006-11-02 7:57 ` walter harms
-1 siblings, 0 replies; 17+ messages in thread
From: walter harms @ 2006-11-02 7:57 UTC (permalink / raw)
To: tipc-discussion, kernel-janitors, netdev
Florian Westphal wrote:
> walter harms <wharms@bfs.de> wrote:
>> These line
>> + strcpy(bcbearer->media.name, "tipc-multicast");
>> i gues that means tipc_bclink_name ?
>
> The idea was to change how things are done, not _what_ is being done.
>
>> an even more secure version could be like this:
>>
>> strncpy(bcbearer->media.name,sizeof(bcbearer->media.name),tipc_bclink_name);
>
> Ugh, please, no. The size of src is known in all cases; there is
> absoluty no point in using str(n|l)cpy here.
>
>> (in case someone ever changes the size of cbearer->media.name or tipc_bclink_name and the hope
>> that wchat_t will never reach the kernel)
>
> In this case 'someone' should be really hurt, don't you think?
>
hi florian,
i am on the side of error, the code increase is marginal and the speed penalty also, so why not ?
you make sure that an overflow may never happen, and the rest in name gets zeroed.
The problem is that when the error occurs it may be later than the actual changeset.
NTL it is an hint, and if you feel ok with it and the maintainer has no objects i have no problems either.
re,
wh
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [KJ] [tipc-discussion] [patch] net/tipc: sprintf/strcpy conversion
@ 2006-11-02 7:57 ` walter harms
0 siblings, 0 replies; 17+ messages in thread
From: walter harms @ 2006-11-02 7:57 UTC (permalink / raw)
To: tipc-discussion, kernel-janitors, netdev
Florian Westphal wrote:
> walter harms <wharms@bfs.de> wrote:
>> These line
>> + strcpy(bcbearer->media.name, "tipc-multicast");
>> i gues that means tipc_bclink_name ?
>
> The idea was to change how things are done, not _what_ is being done.
>
>> an even more secure version could be like this:
>>
>> strncpy(bcbearer->media.name,sizeof(bcbearer->media.name),tipc_bclink_name);
>
> Ugh, please, no. The size of src is known in all cases; there is
> absoluty no point in using str(n|l)cpy here.
>
>> (in case someone ever changes the size of cbearer->media.name or tipc_bclink_name and the hope
>> that wchat_t will never reach the kernel)
>
> In this case 'someone' should be really hurt, don't you think?
>
hi florian,
i am on the side of error, the code increase is marginal and the speed penalty also, so why not ?
you make sure that an overflow may never happen, and the rest in name gets zeroed.
The problem is that when the error occurs it may be later than the actual changeset.
NTL it is an hint, and if you feel ok with it and the maintainer has no objects i have no problems either.
re,
wh
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [KJ] [tipc-discussion] [patch] net/tipc:sprintf/strcpy
2006-11-02 6:26 ` [tipc-discussion] [KJ] [patch] net/tipc: sprintf/strcpy conversion Florian Westphal
(?)
(?)
@ 2006-11-16 7:28 ` Randy Dunlap
-1 siblings, 0 replies; 17+ messages in thread
From: Randy Dunlap @ 2006-11-16 7:28 UTC (permalink / raw)
To: kernel-janitors
On Thu, 2 Nov 2006 13:23:09 -0800 Horvath, Elmer wrote:
> Hi,
>
> Good observations. My comments in <Elmer> </Elmer> way down below.
>
> What is 'kernel-janitors@lists.osdl.org' ?
Mailing list for kernel-janitors work.
See http://www.kerneljanitors.org/ for info.
> <Elmer> If you want to be 'secure', as you say, then strlcpy() is a
> better choice than strncpy() to guarantee that you will have a
> null-terminated string in the destination address (as far as I
> understand the subtle differences). strncpy() will result in a non-null
> terminated octet-string if the source string is too long. Any future
> string operations may cause problems that are harder to trace.
>
> Is strlcpy() available on all implementations? I once compiled code on
> an architecture that did not support it and had to use strncpy() with an
> explicit assignment bloating code:
> dest_string[last_octet] = '\0';
> </Elmer>
(1-2 minutes of grep:)
Yes, strlcpy() is available on all Linux architectures.
An arch. can provide a specific version of it and also
#define __HAVE_ARCH_STRLCPY
If that's not done, then the generic version in lib/string.c
will be used.
---
~Randy
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [KJ] [patch] net/tipc: sprintf/strcpy conversion
2006-11-01 14:06 ` Florian Westphal
@ 2006-11-03 0:09 ` Alexey Dobriyan
-1 siblings, 0 replies; 17+ messages in thread
From: Alexey Dobriyan @ 2006-11-03 0:09 UTC (permalink / raw)
To: kernel-janitors, tipc-discussion, netdev
On Wed, Nov 01, 2006 at 03:06:24PM +0100, Florian Westphal wrote:
> convert sprintf(a,b) to strcpy(a,b). Make tipc_bclink_name[] const.
Ahhh, I missed the start of threads.
Patch is useless because it changes one unbounded string function into
another unbounded string function.
> --- a/net/tipc/bcast.c
> +++ b/net/tipc/bcast.c
> @@ -790,7 +790,7 @@ int tipc_bclink_init(void)
> INIT_LIST_HEAD(&bcbearer->bearer.cong_links);
> bcbearer->bearer.media = &bcbearer->media;
> bcbearer->media.send_msg = tipc_bcbearer_send;
> - sprintf(bcbearer->media.name, "tipc-multicast");
> + strcpy(bcbearer->media.name, "tipc-multicast");
>
> bcl = &bclink->link;
> memset(bclink, 0, sizeof(struct bclink));
> @@ -802,7 +802,7 @@ int tipc_bclink_init(void)
> tipc_link_set_queue_limits(bcl, BCLINK_WIN_DEFAULT);
> bcl->b_ptr = &bcbearer->bearer;
> bcl->state = WORKING_WORKING;
> - sprintf(bcl->name, tipc_bclink_name);
> + strcpy(bcl->name, tipc_bclink_name);
>
> if (BCLINK_LOG_BUF_SIZE) {
> char *pb = kmalloc(BCLINK_LOG_BUF_SIZE, GFP_ATOMIC);
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -667,7 +667,7 @@ struct sk_buff *tipc_node_get_links(cons
> link_info.dest = tipc_own_addr & 0xfffff00;
> link_info.dest = htonl(link_info.dest);
> link_info.up = htonl(1);
> - sprintf(link_info.str, tipc_bclink_name);
> + strcpy(link_info.str, tipc_bclink_name);
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [KJ] [patch] net/tipc: sprintf/strcpy conversion
@ 2006-11-03 0:09 ` Alexey Dobriyan
0 siblings, 0 replies; 17+ messages in thread
From: Alexey Dobriyan @ 2006-11-03 0:09 UTC (permalink / raw)
To: kernel-janitors, tipc-discussion, netdev
On Wed, Nov 01, 2006 at 03:06:24PM +0100, Florian Westphal wrote:
> convert sprintf(a,b) to strcpy(a,b). Make tipc_bclink_name[] const.
Ahhh, I missed the start of threads.
Patch is useless because it changes one unbounded string function into
another unbounded string function.
> --- a/net/tipc/bcast.c
> +++ b/net/tipc/bcast.c
> @@ -790,7 +790,7 @@ int tipc_bclink_init(void)
> INIT_LIST_HEAD(&bcbearer->bearer.cong_links);
> bcbearer->bearer.media = &bcbearer->media;
> bcbearer->media.send_msg = tipc_bcbearer_send;
> - sprintf(bcbearer->media.name, "tipc-multicast");
> + strcpy(bcbearer->media.name, "tipc-multicast");
>
> bcl = &bclink->link;
> memset(bclink, 0, sizeof(struct bclink));
> @@ -802,7 +802,7 @@ int tipc_bclink_init(void)
> tipc_link_set_queue_limits(bcl, BCLINK_WIN_DEFAULT);
> bcl->b_ptr = &bcbearer->bearer;
> bcl->state = WORKING_WORKING;
> - sprintf(bcl->name, tipc_bclink_name);
> + strcpy(bcl->name, tipc_bclink_name);
>
> if (BCLINK_LOG_BUF_SIZE) {
> char *pb = kmalloc(BCLINK_LOG_BUF_SIZE, GFP_ATOMIC);
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -667,7 +667,7 @@ struct sk_buff *tipc_node_get_links(cons
> link_info.dest = tipc_own_addr & 0xfffff00;
> link_info.dest = htonl(link_info.dest);
> link_info.up = htonl(1);
> - sprintf(link_info.str, tipc_bclink_name);
> + strcpy(link_info.str, tipc_bclink_name);
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [KJ] [patch] net/tipc: sprintf/strcpy conversion
2006-11-03 0:09 ` Alexey Dobriyan
@ 2006-11-03 10:50 ` Hagen Paul Pfeifer
-1 siblings, 0 replies; 17+ messages in thread
From: Hagen Paul Pfeifer @ 2006-11-03 10:50 UTC (permalink / raw)
To: Alexey Dobriyan; +Cc: kernel-janitors, tipc-discussion, netdev
* Alexey Dobriyan | 2006-11-03 03:09:05 [+0300]:
>On Wed, Nov 01, 2006 at 03:06:24PM +0100, Florian Westphal wrote:
>> convert sprintf(a,b) to strcpy(a,b). Make tipc_bclink_name[] const.
>
>Ahhh, I missed the start of threads.
>
>Patch is useless because it changes one unbounded string function into
>another unbounded string function.
The discussion in this thread is really back-breaking!
1. To make tipc_bclink_name const there is absolutly no objection
2. Replace sprintf with strcpy
a) First of all: If you _copy_ a string then use also strCPY()
Thats a question of good style!
b) If the compiler is smart enough, he realize that you want to copy
a string and replace the sprintf call with a
pushl %ebx
call strcpy
Surprise - Surprise!
Assumed you use gcc with -Os or -O2! Don't know how icc handle this
case. If you compile without optimization you save at least a
"repz movsb %ds:(%esi),%es:(%edi)" instruction.
c) Last but not least I read all the time this patch doesn't introduce
bounds-checking. This isn't a argument because the author is aware of
the destination length of the buffer. BTW: grep for (sprintf|strcpy) in
/usr/src/linux and be surprised how unsecure the kernel is (thats
ironical).
This patch is 100% OK!
HGN
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [KJ] [patch] net/tipc: sprintf/strcpy conversion
@ 2006-11-03 10:50 ` Hagen Paul Pfeifer
0 siblings, 0 replies; 17+ messages in thread
From: Hagen Paul Pfeifer @ 2006-11-03 10:50 UTC (permalink / raw)
To: Alexey Dobriyan; +Cc: kernel-janitors, tipc-discussion, netdev
* Alexey Dobriyan | 2006-11-03 03:09:05 [+0300]:
>On Wed, Nov 01, 2006 at 03:06:24PM +0100, Florian Westphal wrote:
>> convert sprintf(a,b) to strcpy(a,b). Make tipc_bclink_name[] const.
>
>Ahhh, I missed the start of threads.
>
>Patch is useless because it changes one unbounded string function into
>another unbounded string function.
The discussion in this thread is really back-breaking!
1. To make tipc_bclink_name const there is absolutly no objection
2. Replace sprintf with strcpy
a) First of all: If you _copy_ a string then use also strCPY()
Thats a question of good style!
b) If the compiler is smart enough, he realize that you want to copy
a string and replace the sprintf call with a
pushl %ebx
call strcpy
Surprise - Surprise!
Assumed you use gcc with -Os or -O2! Don't know how icc handle this
case. If you compile without optimization you save at least a
"repz movsb %ds:(%esi),%es:(%edi)" instruction.
c) Last but not least I read all the time this patch doesn't introduce
bounds-checking. This isn't a argument because the author is aware of
the destination length of the buffer. BTW: grep for (sprintf|strcpy) in
/usr/src/linux and be surprised how unsecure the kernel is (thats
ironical).
This patch is 100% OK!
HGN
^ permalink raw reply [flat|nested] 17+ messages in thread