* [PATCH] task name handling in proc fs
@ 2004-07-01 22:05 Mike Kravetz
2004-07-01 22:19 ` Andrew Morton
0 siblings, 1 reply; 11+ messages in thread
From: Mike Kravetz @ 2004-07-01 22:05 UTC (permalink / raw)
To: akpm, viro, linux-kernel
We've seen the the top command segfault when dealing with a badly (very
badly) formed task name obtained from a 'stat' file in proc fs. Upon
further examination, it appears that the task name could be updated at
the same time it is handed off to sprintf in proc_pid_stat(). Now one
could argue that top should be more intelligent and deal with these
badly formed names. However, I think it's bad to be passing strings that
could be changing to sprintf within the kernel. I'm pretty sure sprintf
expects the character strings to be static. Below is a patch to address
this code and one other place dealing with task names in the same file.
--
Mike
diff -Naur linux-2.6.7/fs/proc/array.c linux-2.6.7.ptest/fs/proc/array.c
--- linux-2.6.7/fs/proc/array.c Wed Jun 16 05:19:36 2004
+++ linux-2.6.7.ptest/fs/proc/array.c Thu Jul 1 17:44:14 2004
@@ -97,14 +97,14 @@
name++;
i--;
*buf = c;
- if (!c)
+ if (!*buf)
break;
- if (c == '\\') {
- buf[1] = c;
+ if (*buf == '\\') {
+ buf[1] = *buf;
buf += 2;
continue;
}
- if (c == '\n') {
+ if (*buf == '\n') {
buf[0] = '\\';
buf[1] = 'n';
buf += 2;
@@ -308,14 +308,11 @@
int num_threads = 0;
struct mm_struct *mm;
unsigned long long start_time;
+ char tname[sizeof(task->comm)];
state = *get_task_state(task);
vsize = eip = esp = 0;
- task_lock(task);
- mm = task->mm;
- if(mm)
- mm = mmgrab(mm);
- task_unlock(task);
+ mm = get_task_mm(task);
if (mm) {
down_read(&mm->mmap_sem);
vsize = task_vsize(mm);
@@ -357,11 +354,15 @@
/* Temporary variable needed for gcc-2.96 */
start_time = jiffies_64_to_clock_t(task->start_time - INITIAL_JIFFIES);
+ /* Make a static copy of task name for sprintf */
+ memcpy(tname, task->comm, sizeof(tname));
+ tname[sizeof(tname)-1] = '\0';
+
res = sprintf(buffer,"%d (%s) %c %d %d %d %d %d %lu %lu \
%lu %lu %lu %lu %lu %ld %ld %ld %ld %d %ld %llu %lu %ld %lu %lu %lu %lu %lu \
%lu %lu %lu %lu %lu %lu %lu %lu %d %d %lu %lu\n",
task->pid,
- task->comm,
+ tname,
state,
ppid,
pgid,
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] task name handling in proc fs
2004-07-01 22:05 [PATCH] task name handling in proc fs Mike Kravetz
@ 2004-07-01 22:19 ` Andrew Morton
2004-07-01 22:42 ` Mike Kravetz
0 siblings, 1 reply; 11+ messages in thread
From: Andrew Morton @ 2004-07-01 22:19 UTC (permalink / raw)
To: Mike Kravetz; +Cc: viro, linux-kernel
Mike Kravetz <kravetz@us.ibm.com> wrote:
>
> --- linux-2.6.7/fs/proc/array.c Wed Jun 16 05:19:36 2004
> +++ linux-2.6.7.ptest/fs/proc/array.c Thu Jul 1 17:44:14 2004
> @@ -97,14 +97,14 @@
> name++;
> i--;
> *buf = c;
> - if (!c)
> + if (!*buf)
> break;
> - if (c == '\\') {
> - buf[1] = c;
> + if (*buf == '\\') {
> + buf[1] = *buf;
> buf += 2;
> continue;
> }
> - if (c == '\n') {
> + if (*buf == '\n') {
> buf[0] = '\\';
> buf[1] = 'n';
> buf += 2;
What is this code for?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] task name handling in proc fs
2004-07-01 22:19 ` Andrew Morton
@ 2004-07-01 22:42 ` Mike Kravetz
2004-07-01 23:03 ` Andrew Morton
2004-07-02 1:27 ` Andrew Rodland
0 siblings, 2 replies; 11+ messages in thread
From: Mike Kravetz @ 2004-07-01 22:42 UTC (permalink / raw)
To: Andrew Morton; +Cc: viro, linux-kernel
On Thu, Jul 01, 2004 at 03:19:35PM -0700, Andrew Morton wrote:
> Mike Kravetz <kravetz@us.ibm.com> wrote:
> >
> > --- linux-2.6.7/fs/proc/array.c Wed Jun 16 05:19:36 2004
> > +++ linux-2.6.7.ptest/fs/proc/array.c Thu Jul 1 17:44:14 2004
> > @@ -97,14 +97,14 @@
> > name++;
> > i--;
> > *buf = c;
> > - if (!c)
> > + if (!*buf)
> > break;
> > - if (c == '\\') {
> > - buf[1] = c;
> > + if (*buf == '\\') {
> > + buf[1] = *buf;
> > buf += 2;
> > continue;
> > }
> > - if (c == '\n') {
> > + if (*buf == '\n') {
> > buf[0] = '\\';
> > buf[1] = 'n';
> > buf += 2;
>
> What is this code for?
The code is copying the task name from 'c' to 'buf' one character
at a time. It is then 'post processing' the characters. Currently,
the post processing is based on the value of c which is part of the
source string (task->curr). However, it is possible for the source
string to change during this copy (think exec). In such a case I
think it is better to base the 'post processing' on the character
that already has been safely been copied to the target string rather
than the character in the source string which might have changed.
--
Mike
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] task name handling in proc fs
2004-07-01 22:42 ` Mike Kravetz
@ 2004-07-01 23:03 ` Andrew Morton
2004-07-01 23:38 ` Mike Kravetz
2004-07-07 21:52 ` Mike Kravetz
2004-07-02 1:27 ` Andrew Rodland
1 sibling, 2 replies; 11+ messages in thread
From: Andrew Morton @ 2004-07-01 23:03 UTC (permalink / raw)
To: Mike Kravetz; +Cc: viro, linux-kernel
Mike Kravetz <kravetz@us.ibm.com> wrote:
>
> On Thu, Jul 01, 2004 at 03:19:35PM -0700, Andrew Morton wrote:
> > Mike Kravetz <kravetz@us.ibm.com> wrote:
> > >
> > > --- linux-2.6.7/fs/proc/array.c Wed Jun 16 05:19:36 2004
> > > +++ linux-2.6.7.ptest/fs/proc/array.c Thu Jul 1 17:44:14 2004
> > > @@ -97,14 +97,14 @@
> > > name++;
> > > i--;
> > > *buf = c;
> > > - if (!c)
> > > + if (!*buf)
> > > break;
> > > - if (c == '\\') {
> > > - buf[1] = c;
> > > + if (*buf == '\\') {
> > > + buf[1] = *buf;
> > > buf += 2;
> > > continue;
> > > }
> > > - if (c == '\n') {
> > > + if (*buf == '\n') {
> > > buf[0] = '\\';
> > > buf[1] = 'n';
> > > buf += 2;
> >
> > What is this code for?
>
> The code is copying the task name from 'c' to 'buf' one character
> at a time. It is then 'post processing' the characters. Currently,
> the post processing is based on the value of c which is part of the
> source string (task->curr). However, it is possible for the source
> string to change during this copy (think exec). In such a case I
> think it is better to base the 'post processing' on the character
> that already has been safely been copied to the target string rather
> than the character in the source string which might have changed.
But this just makes it a bit less racy than it used to be.
If we need locking around task->comm (and it seems that we do) then
let's do it.
void get_task_comm(char *buf, struct task_struct *tsk);
void set_task_comm(struct task_struct *tsk, char *buf);
It would be a bit lame to add a new lock for this - probably
task_lock() could be coopted.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] task name handling in proc fs
2004-07-01 23:03 ` Andrew Morton
@ 2004-07-01 23:38 ` Mike Kravetz
2004-07-07 21:52 ` Mike Kravetz
1 sibling, 0 replies; 11+ messages in thread
From: Mike Kravetz @ 2004-07-01 23:38 UTC (permalink / raw)
To: Andrew Morton; +Cc: viro, linux-kernel
On Thu, Jul 01, 2004 at 04:03:35PM -0700, Andrew Morton wrote:
>
> But this just makes it a bit less racy than it used to be.
Agreed!
> If we need locking around task->comm (and it seems that we do) then
> let's do it.
I'm not sure if there is a need/desire for locking. But, I don't
have any proc fs history. What we saw were really badly formed
task names: nulls embedded within strings. My goal was to simply
provide well formed strings (even if they didn't make sense).
I guess the question is 'how important is it to make sure that
data in these files is consistent/accurate when reported?'. If it
is highly important, then we need to consider locking down the
task for the duration of gathering all data we want to display
(not just task->comm). However, my guess is that we would want
to sacrifice this accuracy/consistency to avoid taking locks if
possible. Again, this is just my opinion and I don't have any
history here ... but would be happy to code up any recommendation.
P.S. Note that these code paths already get the task lock once
as a result of calling get_task_mm().
--
Mike
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] task name handling in proc fs
2004-07-01 22:42 ` Mike Kravetz
2004-07-01 23:03 ` Andrew Morton
@ 2004-07-02 1:27 ` Andrew Rodland
1 sibling, 0 replies; 11+ messages in thread
From: Andrew Rodland @ 2004-07-02 1:27 UTC (permalink / raw)
To: linux-kernel
Mike Kravetz wrote:
> On Thu, Jul 01, 2004 at 03:19:35PM -0700, Andrew Morton wrote:
>> Mike Kravetz <kravetz@us.ibm.com> wrote:
>> >
>> > --- linux-2.6.7/fs/proc/array.c Wed Jun 16 05:19:36 2004
>> > +++ linux-2.6.7.ptest/fs/proc/array.c Thu Jul 1 17:44:14 2004
>> > @@ -97,14 +97,14 @@
>> > name++;
>> > i--;
>> > *buf = c;
>> > - if (!c)
>> > + if (!*buf)
>> > break;
>> > - if (c == '\\') {
>> > - buf[1] = c;
>> > + if (*buf == '\\') {
>> > + buf[1] = *buf;
>> > buf += 2;
>> > continue;
>> > }
>> > - if (c == '\n') {
>> > + if (*buf == '\n') {
>> > buf[0] = '\\';
>> > buf[1] = 'n';
>> > buf += 2;
>>
>> What is this code for?
>
> The code is copying the task name from 'c' to 'buf' one character
> at a time. It is then 'post processing' the characters. Currently,
> the post processing is based on the value of c which is part of the
> source string (task->curr). However, it is possible for the source
> string to change during this copy (think exec).
Except that c is not "part of the source string". The code "c =
*name" (where name starts off pointing to the same place as p->comm) is
executed once and only once per time through the loop. Then it does "*buf =
c". Your change would protect not against a change in "name" (which is
possible), but against a change in "buf" while we're writing to it
(impossible, as long as I'm understanding the proc code correctly).
Not that there is no race here, but that doesn't fix it. What's needed is
either another strcpy or locking around p->comm as suggested by Andrew
Morton.
--Andrew Rodland < arodland@entermail.net > via GMANE
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] task name handling in proc fs
2004-07-01 23:03 ` Andrew Morton
2004-07-01 23:38 ` Mike Kravetz
@ 2004-07-07 21:52 ` Mike Kravetz
2004-07-07 22:11 ` Andrew Morton
1 sibling, 1 reply; 11+ messages in thread
From: Mike Kravetz @ 2004-07-07 21:52 UTC (permalink / raw)
To: Andrew Morton; +Cc: viro, linux-kernel
On Thu, Jul 01, 2004 at 04:03:35PM -0700, Andrew Morton wrote:
>
> If we need locking around task->comm (and it seems that we do) then
> let's do it.
>
> void get_task_comm(char *buf, struct task_struct *tsk);
> void set_task_comm(struct task_struct *tsk, char *buf);
>
> It would be a bit lame to add a new lock for this - probably
> task_lock() could be coopted.
OK - Here is a patch to implement/use the routines above. I couldn't
decide on a good location (file) for them so I put them in 'fs/exec/c'
which is where the existing set_task_comm functionality resides. In
addition, I took the liberty of replacing the code in 'fs/proc/array.c'
that replicates the code in get_task_mm() with a simple call to the
routine.
--
Mike
diff -Naur linux-2.6.7/fs/exec.c linux-2.6.7.ptest2/fs/exec.c
--- linux-2.6.7/fs/exec.c Wed Jun 16 05:19:13 2004
+++ linux-2.6.7.ptest2/fs/exec.c Wed Jul 7 21:46:20 2004
@@ -786,10 +786,33 @@
spin_unlock(&files->file_lock);
}
+void get_task_comm(char *buf, struct task_struct *tsk)
+{
+ /* buf must be at least sizeof(tsk->comm) in size */
+ task_lock(tsk);
+ memcpy(buf, tsk->comm, sizeof(tsk->comm));
+ task_unlock(tsk);
+}
+
+void set_task_comm(struct task_struct *tsk, char *name)
+{
+ int i, ch;
+
+ task_lock(tsk);
+ for (i=0; (ch = *(name++)) != '\0';) {
+ if (ch == '/')
+ i = 0;
+ else
+ if (i < (sizeof(tsk->comm) - 1))
+ tsk->comm[i++] = ch;
+ }
+ tsk->comm[i] = '\0';
+ task_unlock(tsk);
+}
+
int flush_old_exec(struct linux_binprm * bprm)
{
- char * name;
- int i, ch, retval;
+ int retval;
struct files_struct *files;
/*
@@ -826,15 +849,8 @@
if (current->euid == current->uid && current->egid == current->gid)
current->mm->dumpable = 1;
- name = bprm->filename;
- for (i=0; (ch = *(name++)) != '\0';) {
- if (ch == '/')
- i = 0;
- else
- if (i < 15)
- current->comm[i++] = ch;
- }
- current->comm[i] = '\0';
+
+ set_task_comm(current, bprm->filename);
flush_thread();
diff -Naur linux-2.6.7/fs/proc/array.c linux-2.6.7.ptest2/fs/proc/array.c
--- linux-2.6.7/fs/proc/array.c Wed Jun 16 05:19:36 2004
+++ linux-2.6.7.ptest2/fs/proc/array.c Wed Jul 7 17:41:28 2004
@@ -88,10 +88,13 @@
{
int i;
char * name;
+ char tcomm[sizeof(p->comm)];
+
+ get_task_comm(tcomm, p);
ADDBUF(buf, "Name:\t");
- name = p->comm;
- i = sizeof(p->comm);
+ name = tcomm;
+ i = sizeof(tcomm);
do {
unsigned char c = *name;
name++;
@@ -308,14 +311,11 @@
int num_threads = 0;
struct mm_struct *mm;
unsigned long long start_time;
+ char tcomm[sizeof(task->comm)];
state = *get_task_state(task);
vsize = eip = esp = 0;
- task_lock(task);
- mm = task->mm;
- if(mm)
- mm = mmgrab(mm);
- task_unlock(task);
+ mm = get_task_mm(task);
if (mm) {
down_read(&mm->mmap_sem);
vsize = task_vsize(mm);
@@ -324,6 +324,7 @@
up_read(&mm->mmap_sem);
}
+ get_task_comm(tcomm, task);
wchan = get_wchan(task);
sigemptyset(&sigign);
@@ -361,7 +362,7 @@
%lu %lu %lu %lu %lu %ld %ld %ld %ld %d %ld %llu %lu %ld %lu %lu %lu %lu %lu \
%lu %lu %lu %lu %lu %lu %lu %lu %d %d %lu %lu\n",
task->pid,
- task->comm,
+ tcomm,
state,
ppid,
pgid,
diff -Naur linux-2.6.7/include/linux/sched.h linux-2.6.7.ptest2/include/linux/sched.h
--- linux-2.6.7/include/linux/sched.h Wed Jun 16 05:18:57 2004
+++ linux-2.6.7.ptest2/include/linux/sched.h Tue Jul 6 22:39:25 2004
@@ -868,6 +868,9 @@
extern long do_fork(unsigned long, unsigned long, struct pt_regs *, unsigned long, int __user *, int __user *);
extern struct task_struct * copy_process(unsigned long, unsigned long, struct pt_regs *, unsigned long, int __user *, int __user *);
+extern void set_task_comm(struct task_struct *, char *);
+extern void get_task_comm(char *, struct task_struct *);
+
#ifdef CONFIG_SMP
extern void wait_task_inactive(task_t * p);
#else
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] task name handling in proc fs
2004-07-07 21:52 ` Mike Kravetz
@ 2004-07-07 22:11 ` Andrew Morton
2004-07-07 23:35 ` Mike Kravetz
0 siblings, 1 reply; 11+ messages in thread
From: Andrew Morton @ 2004-07-07 22:11 UTC (permalink / raw)
To: Mike Kravetz; +Cc: viro, linux-kernel
Mike Kravetz <kravetz@us.ibm.com> wrote:
>
> +void set_task_comm(struct task_struct *tsk, char *name)
> +{
> + int i, ch;
> +
> + task_lock(tsk);
> + for (i=0; (ch = *(name++)) != '\0';) {
> + if (ch == '/')
> + i = 0;
> + else
> + if (i < (sizeof(tsk->comm) - 1))
> + tsk->comm[i++] = ch;
> + }
> + tsk->comm[i] = '\0';
> + task_unlock(tsk);
> +}
I don't think the basename logic should be in this function. Only one
caller needs it, and if we later try to use this function to set
current->comm for per-cpu kernel threads, it will mangle the text.
root 2 0.0 0.0 0 0 ? SW Jul06 0:00 [migration/0]
root 3 0.0 0.0 0 0 ? SWN Jul06 0:00 [ksoftirqd/0]
root 4 0.0 0.0 0 0 ? SW Jul06 0:00 [migration/1]
root 5 0.0 0.0 0 0 ? SWN Jul06 0:00 [ksoftirqd/1]
root 6 0.0 0.0 0 0 ? SW Jul06 0:00 [migration/2]
root 7 0.0 0.0 0 0 ? SWN Jul06 0:00 [ksoftirqd/2]
root 8 0.0 0.0 0 0 ? SW Jul06 0:00 [migration/3]
root 9 0.0 0.0 0 0 ? SWN Jul06 0:00 [ksoftirqd/3]
We probably won't actually _do_ that, since kthread_create() uses vsnprintf(),
but pushing code which is specific to one caller into a library function
seems wrong...
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] task name handling in proc fs
2004-07-07 22:11 ` Andrew Morton
@ 2004-07-07 23:35 ` Mike Kravetz
2004-07-08 2:32 ` Paul Jackson
0 siblings, 1 reply; 11+ messages in thread
From: Mike Kravetz @ 2004-07-07 23:35 UTC (permalink / raw)
To: Andrew Morton; +Cc: viro, linux-kernel
On Wed, Jul 07, 2004 at 03:11:34PM -0700, Andrew Morton wrote:
>
> I don't think the basename logic should be in this function.
>
OK - Here is another version.
--
Mike
diff -Naur linux-2.6.7/fs/exec.c linux-2.6.7.ptest2/fs/exec.c
--- linux-2.6.7/fs/exec.c Wed Jun 16 05:19:13 2004
+++ linux-2.6.7.ptest2/fs/exec.c Wed Jul 7 23:30:47 2004
@@ -786,11 +786,35 @@
spin_unlock(&files->file_lock);
}
+void get_task_comm(char *buf, struct task_struct *tsk)
+{
+ /* buf must be at least sizeof(tsk->comm) in size */
+ task_lock(tsk);
+ memcpy(buf, tsk->comm, sizeof(tsk->comm));
+ task_unlock(tsk);
+}
+
+void set_task_comm(struct task_struct *tsk, char *buf)
+{
+ /* buf must be null terminated and <= sizeof(tsk->comm) */
+ int i;
+
+ task_lock(tsk);
+ for(i=0; i<sizeof(tsk->comm); i++) {
+ tsk->comm[i] = *buf++;
+ if (!tsk->comm[i])
+ break;
+ }
+ tsk->comm[sizeof(tsk->comm)-1] = '\0'; /* just in case */
+ task_unlock(tsk);
+}
+
int flush_old_exec(struct linux_binprm * bprm)
{
char * name;
int i, ch, retval;
struct files_struct *files;
+ char tcomm[sizeof(current->comm)];
/*
* Make sure we have a private signal table and that
@@ -826,15 +850,18 @@
if (current->euid == current->uid && current->egid == current->gid)
current->mm->dumpable = 1;
+
name = bprm->filename;
for (i=0; (ch = *(name++)) != '\0';) {
if (ch == '/')
i = 0;
else
- if (i < 15)
- current->comm[i++] = ch;
+ if (i < (sizeof(tcomm) - 1))
+ tcomm[i++] = ch;
}
- current->comm[i] = '\0';
+ tcomm[i] = '\0';
+
+ set_task_comm(current, tcomm);
flush_thread();
diff -Naur linux-2.6.7/fs/proc/array.c linux-2.6.7.ptest2/fs/proc/array.c
--- linux-2.6.7/fs/proc/array.c Wed Jun 16 05:19:36 2004
+++ linux-2.6.7.ptest2/fs/proc/array.c Wed Jul 7 17:41:28 2004
@@ -88,10 +88,13 @@
{
int i;
char * name;
+ char tcomm[sizeof(p->comm)];
+
+ get_task_comm(tcomm, p);
ADDBUF(buf, "Name:\t");
- name = p->comm;
- i = sizeof(p->comm);
+ name = tcomm;
+ i = sizeof(tcomm);
do {
unsigned char c = *name;
name++;
@@ -308,14 +311,11 @@
int num_threads = 0;
struct mm_struct *mm;
unsigned long long start_time;
+ char tcomm[sizeof(task->comm)];
state = *get_task_state(task);
vsize = eip = esp = 0;
- task_lock(task);
- mm = task->mm;
- if(mm)
- mm = mmgrab(mm);
- task_unlock(task);
+ mm = get_task_mm(task);
if (mm) {
down_read(&mm->mmap_sem);
vsize = task_vsize(mm);
@@ -324,6 +324,7 @@
up_read(&mm->mmap_sem);
}
+ get_task_comm(tcomm, task);
wchan = get_wchan(task);
sigemptyset(&sigign);
@@ -361,7 +362,7 @@
%lu %lu %lu %lu %lu %ld %ld %ld %ld %d %ld %llu %lu %ld %lu %lu %lu %lu %lu \
%lu %lu %lu %lu %lu %lu %lu %lu %d %d %lu %lu\n",
task->pid,
- task->comm,
+ tcomm,
state,
ppid,
pgid,
diff -Naur linux-2.6.7/include/linux/sched.h linux-2.6.7.ptest2/include/linux/sched.h
--- linux-2.6.7/include/linux/sched.h Wed Jun 16 05:18:57 2004
+++ linux-2.6.7.ptest2/include/linux/sched.h Tue Jul 6 22:39:25 2004
@@ -868,6 +868,9 @@
extern long do_fork(unsigned long, unsigned long, struct pt_regs *, unsigned long, int __user *, int __user *);
extern struct task_struct * copy_process(unsigned long, unsigned long, struct pt_regs *, unsigned long, int __user *, int __user *);
+extern void set_task_comm(struct task_struct *, char *);
+extern void get_task_comm(char *, struct task_struct *);
+
#ifdef CONFIG_SMP
extern void wait_task_inactive(task_t * p);
#else
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] task name handling in proc fs
2004-07-07 23:35 ` Mike Kravetz
@ 2004-07-08 2:32 ` Paul Jackson
2004-07-08 17:01 ` Mike Kravetz
0 siblings, 1 reply; 11+ messages in thread
From: Paul Jackson @ 2004-07-08 2:32 UTC (permalink / raw)
To: Mike Kravetz; +Cc: akpm, viro, linux-kernel
+void set_task_comm(struct task_struct *tsk, char *buf)
+{ ...
+ for(i=0; i<sizeof(tsk->comm); i++) {
+ tsk->comm[i] = *buf++;
+ if (!tsk->comm[i])
+ break;
+ }
+ tsk->comm[sizeof(tsk->comm)-1] = '\0'; /* just in case */
That code fragment looks to me like:
strlcpy (tsk->comm, buf, sizeof(tsk->comm));
Unless I'm missing something, I'd prefer 'strlcpy'.
--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <pj@sgi.com> 1.650.933.1373
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] task name handling in proc fs
2004-07-08 2:32 ` Paul Jackson
@ 2004-07-08 17:01 ` Mike Kravetz
0 siblings, 0 replies; 11+ messages in thread
From: Mike Kravetz @ 2004-07-08 17:01 UTC (permalink / raw)
To: Paul Jackson, akpm; +Cc: viro, linux-kernel
On Wed, Jul 07, 2004 at 07:32:22PM -0700, Paul Jackson wrote:
>
> Unless I'm missing something, I'd prefer 'strlcpy'.
>
Thanks Paul. strlcpy isn't one of my commonly used routines, so
it's use didn't occur to me. Here is the patch using strlcpy.
--
Mike
diff -Naur linux-2.6.7/fs/exec.c linux-2.6.7.ptest2/fs/exec.c
--- linux-2.6.7/fs/exec.c Wed Jun 16 05:19:13 2004
+++ linux-2.6.7.ptest2/fs/exec.c Thu Jul 8 16:57:36 2004
@@ -786,11 +786,27 @@
spin_unlock(&files->file_lock);
}
+void get_task_comm(char *buf, struct task_struct *tsk)
+{
+ /* buf must be at least sizeof(tsk->comm) in size */
+ task_lock(tsk);
+ memcpy(buf, tsk->comm, sizeof(tsk->comm));
+ task_unlock(tsk);
+}
+
+void set_task_comm(struct task_struct *tsk, char *buf)
+{
+ task_lock(tsk);
+ strlcpy(tsk->comm, buf, sizeof(tsk->comm));
+ task_unlock(tsk);
+}
+
int flush_old_exec(struct linux_binprm * bprm)
{
char * name;
int i, ch, retval;
struct files_struct *files;
+ char tcomm[sizeof(current->comm)];
/*
* Make sure we have a private signal table and that
@@ -826,15 +842,17 @@
if (current->euid == current->uid && current->egid == current->gid)
current->mm->dumpable = 1;
+
name = bprm->filename;
for (i=0; (ch = *(name++)) != '\0';) {
if (ch == '/')
i = 0;
else
- if (i < 15)
- current->comm[i++] = ch;
+ if (i < (sizeof(tcomm) - 1))
+ tcomm[i++] = ch;
}
- current->comm[i] = '\0';
+ tcomm[i] = '\0';
+ set_task_comm(current, tcomm);
flush_thread();
diff -Naur linux-2.6.7/fs/proc/array.c linux-2.6.7.ptest2/fs/proc/array.c
--- linux-2.6.7/fs/proc/array.c Wed Jun 16 05:19:36 2004
+++ linux-2.6.7.ptest2/fs/proc/array.c Wed Jul 7 17:41:28 2004
@@ -88,10 +88,13 @@
{
int i;
char * name;
+ char tcomm[sizeof(p->comm)];
+
+ get_task_comm(tcomm, p);
ADDBUF(buf, "Name:\t");
- name = p->comm;
- i = sizeof(p->comm);
+ name = tcomm;
+ i = sizeof(tcomm);
do {
unsigned char c = *name;
name++;
@@ -308,14 +311,11 @@
int num_threads = 0;
struct mm_struct *mm;
unsigned long long start_time;
+ char tcomm[sizeof(task->comm)];
state = *get_task_state(task);
vsize = eip = esp = 0;
- task_lock(task);
- mm = task->mm;
- if(mm)
- mm = mmgrab(mm);
- task_unlock(task);
+ mm = get_task_mm(task);
if (mm) {
down_read(&mm->mmap_sem);
vsize = task_vsize(mm);
@@ -324,6 +324,7 @@
up_read(&mm->mmap_sem);
}
+ get_task_comm(tcomm, task);
wchan = get_wchan(task);
sigemptyset(&sigign);
@@ -361,7 +362,7 @@
%lu %lu %lu %lu %lu %ld %ld %ld %ld %d %ld %llu %lu %ld %lu %lu %lu %lu %lu \
%lu %lu %lu %lu %lu %lu %lu %lu %d %d %lu %lu\n",
task->pid,
- task->comm,
+ tcomm,
state,
ppid,
pgid,
diff -Naur linux-2.6.7/include/linux/sched.h linux-2.6.7.ptest2/include/linux/sched.h
--- linux-2.6.7/include/linux/sched.h Wed Jun 16 05:18:57 2004
+++ linux-2.6.7.ptest2/include/linux/sched.h Tue Jul 6 22:39:25 2004
@@ -868,6 +868,9 @@
extern long do_fork(unsigned long, unsigned long, struct pt_regs *, unsigned long, int __user *, int __user *);
extern struct task_struct * copy_process(unsigned long, unsigned long, struct pt_regs *, unsigned long, int __user *, int __user *);
+extern void set_task_comm(struct task_struct *, char *);
+extern void get_task_comm(char *, struct task_struct *);
+
#ifdef CONFIG_SMP
extern void wait_task_inactive(task_t * p);
#else
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2004-07-08 17:02 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-07-01 22:05 [PATCH] task name handling in proc fs Mike Kravetz
2004-07-01 22:19 ` Andrew Morton
2004-07-01 22:42 ` Mike Kravetz
2004-07-01 23:03 ` Andrew Morton
2004-07-01 23:38 ` Mike Kravetz
2004-07-07 21:52 ` Mike Kravetz
2004-07-07 22:11 ` Andrew Morton
2004-07-07 23:35 ` Mike Kravetz
2004-07-08 2:32 ` Paul Jackson
2004-07-08 17:01 ` Mike Kravetz
2004-07-02 1:27 ` Andrew Rodland
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox