[Qemu-devel] [PATCH] linux-user: improve fake /proc/self/stat making `ps` not segfault.

[Qemu-devel] [PATCH] linux-user: improve fake /proc/self/stat making `ps` not segfault.

[Fabio Erculiani]

With the current fake /proc/self/stat implementation `ps` is
segfaulting because it expects to read PID and argv[0] as first and
second field respectively, with the latter being enclosed between
backets.

Reproducing is as easy as running: `ps` inside qemu-user chroot
with /proc mounted.

Signed-off-by: Fabio Erculiani <lxnay@sabayon.org>
---
 linux-user/syscall.c |   20 +++++++++++++++-----
 1 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 9ba51bf..f2af5d5 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -4678,14 +4678,24 @@ static int open_self_stat(void *cpu_env, int fd)
       int len;
       uint64_t val = 0;
 
-      if (i == 27) {
-          /* stack bottom */
-          val = start_stack;
+      if (i == 0) {
+        /* pid */
+        snprintf(buf, sizeof(buf), "%"PRId64 " ", getpid());
+      } else if (i == 1) {
+        /* app name */
+        snprintf(buf, sizeof(buf), "(%s) ", ts->bprm->argv[0]);
+      } else if (i == 27) {
+        /* stack bottom */
+        val = start_stack;
+        snprintf(buf, sizeof(buf), "%"PRId64 " ", val);
+      } else {
+        /* for the rest, there is MasterCard */
+        snprintf(buf, sizeof(buf), "0%c", i == 43 ? '\n' : ' ');
       }
-      snprintf(buf, sizeof(buf), "%"PRId64 "%c", val, i == 43 ? '\n' : ' ');
+
       len = strlen(buf);
       if (write(fd, buf, len) != len) {
-          return -1;
+        return -1;
       }
     }
 
-- 
1.7.7

Re: [Qemu-devel] [PATCH] linux-user: improve fake /proc/self/stat making `ps` not segfault.

[Fabio Erculiani]

ts->bprm->argv seems NULL here.
Isn't it supposed to be set?

-- 
Fabio Erculiani

Re: [Qemu-devel] [PATCH] linux-user: improve fake /proc/self/stat making `ps` not segfault.

[Alexander Graf]

On 03.01.2012, at 17:07, Fabio Erculiani wrote:

> ts->bprm->argv seems NULL here.
> Isn't it supposed to be set?

Good question. Maybe we need some other way to fetch argv0 then?


Alex

Re: [Qemu-devel] [PATCH] linux-user: improve fake /proc/self/stat making `ps` not segfault.

[Fabio Erculiani]

On Tue, Jan 3, 2012 at 6:41 PM, Alexander Graf <agraf@suse.de> wrote:
>
> On 03.01.2012, at 17:07, Fabio Erculiani wrote:
>
>> ts->bprm->argv seems NULL here.
>> Isn't it supposed to be set?
>
> Good question. Maybe we need some other way to fetch argv0 then?

or we could leave just an empty string for now -> "()" on the second
field of self/state.
That seems to work with ps as well.
Considering that the whole file doesn't bring any useful info other
than the stack base and pid, we might just wait the next round of
segfaults ;-)

>
>
> Alex
>



-- 
Fabio Erculiani

Re: [Qemu-devel] [PATCH] linux-user: improve fake /proc/self/stat making `ps` not segfault.

[Fabio Erculiani]

Or just using linux_binprm->filename with basename()

-- 
Fabio Erculiani

Re: [Qemu-devel] [PATCH] linux-user: improve fake /proc/self/stat making `ps` not segfault.

[Alexander Graf]

On 03.01.2012, at 19:08, Fabio Erculiani wrote:

> Or just using linux_binprm->filename with basename()

No, that'd be wrong since argv[0] can be different from the actual file name. Also it can be the full path or not depending on what the initiator defined.


Alex

Re: [Qemu-devel] [PATCH] linux-user: improve fake /proc/self/stat making `ps` not segfault.

[Alexander Graf]

On 03.01.2012, at 19:04, Fabio Erculiani wrote:

> On Tue, Jan 3, 2012 at 6:41 PM, Alexander Graf <agraf@suse.de> wrote:
>> 
>> On 03.01.2012, at 17:07, Fabio Erculiani wrote:
>> 
>>> ts->bprm->argv seems NULL here.
>>> Isn't it supposed to be set?
>> 
>> Good question. Maybe we need some other way to fetch argv0 then?
> 
> or we could leave just an empty string for now -> "()" on the second
> field of self/state.
> That seems to work with ps as well.
> Considering that the whole file doesn't bring any useful info other
> than the stack base and pid, we might just wait the next round of
> segfaults ;-)

Hrm. For the sake of simplicity I'd just make target_argv in main.c a global and directly access it. We only ever execute a single process in linux-user at a given time anyway.


Alex

Re: [Qemu-devel] [PATCH] linux-user: improve fake /proc/self/stat making `ps` not segfault.

[Fabio Erculiani]

How about setting ts->bprm->argv = target_argv; ?
I'm not a qemu codebase expert, but if it's always NULL (why is it
NULL?) or can be NULL...

It looks like can be done easily from main.c... without making a
variable global.

-- 
Fabio Erculiani

Re: [Qemu-devel] [PATCH] linux-user: improve fake /proc/self/stat making `ps` not segfault.

[Fabio Erculiani]

Mumble,
that is what happens already...

Let me see why I get NULL here...

-- 
Fabio Erculiani

Re: [Qemu-devel] [PATCH] linux-user: improve fake /proc/self/stat making `ps` not segfault.

[Alexander Graf]

On 03.01.2012, at 19:46, Fabio Erculiani wrote:

> How about setting ts->bprm->argv = target_argv; ?
> I'm not a qemu codebase expert, but if it's always NULL (why is it
> NULL?) or can be NULL...

It should already be set it loader_exec. I don't know why it's NULL there for you. I suppose some debug printfs could help solve this riddle? ;)

Alex

Re: [Qemu-devel] [PATCH] linux-user: improve fake /proc/self/stat making `ps` not segfault.

[Fabio Erculiani]

Yeah, debugging.

Moreover we have this scenario:

$ /bin/cat /proc/self/stat
32297 (cat) ......

I guess we should use basename() anyway...?

-- 
Fabio Erculiani

Re: [Qemu-devel] [PATCH] linux-user: improve fake /proc/self/stat making `ps` not segfault.

[Alexander Graf]

On 03.01.2012, at 19:54, Fabio Erculiani wrote:

> Yeah, debugging.
> 
> Moreover we have this scenario:
> 
> $ /bin/cat /proc/self/stat
> 32297 (cat) ......
> 
> I guess we should use basename() anyway...?

argv[0] can be an arbitrary value passed in through execve. In qemu's linux-user emulation you can even override it manually with the -argv0 command line option. So no, it shouldn't be basename of the file name.


Alex

Re: [Qemu-devel] [PATCH] linux-user: improve fake /proc/self/stat making `ps` not segfault.

[Fabio Erculiani]

Ok, I've found the reason, i guess it's a bug.
target_argv pointer is placed in bprm->argv;
But then target_argv is freed and nullified.

loader_exec should just allocate a new char** and copy target_argv.
I tried that and it worked.

The problem is, where do I free() it? Am i supposed to do it or the
TaskState lifecycle matches the executable (so there is no need to
free() it) ?

-- 
Fabio Erculiani

Re: [Qemu-devel] [PATCH] linux-user: improve fake /proc/self/stat making `ps` not segfault.

[Alexander Graf]

On 03.01.2012, at 20:11, Fabio Erculiani wrote:

> Ok, I've found the reason, i guess it's a bug.
> target_argv pointer is placed in bprm->argv;
> But then target_argv is freed and nullified.
> 
> loader_exec should just allocate a new char** and copy target_argv.
> I tried that and it worked.
> 
> The problem is, where do I free() it? Am i supposed to do it or the
> TaskState lifecycle matches the executable (so there is no need to
> free() it) ?

Can't you just remove the first free()?


Alex

Re: [Qemu-devel] [PATCH] linux-user: improve fake /proc/self/stat making `ps` not segfault.

[Fabio Erculiani]

Done, it all works now ;-) !

-- 
Fabio Erculiani