diff mbox

linux-user: improve fake /proc/self/stat making `ps` not segfault.

Message ID 1325605408-16401-1-git-send-email-lxnay@sabayon.org
State New
Headers show

Commit Message

Fabio Erculiani Jan. 3, 2012, 3:43 p.m. UTC
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(-)

Comments

Fabio Erculiani Jan. 3, 2012, 4:07 p.m. UTC | #1
ts->bprm->argv seems NULL here.
Isn't it supposed to be set?
Alexander Graf Jan. 3, 2012, 5:41 p.m. UTC | #2
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
Fabio Erculiani Jan. 3, 2012, 6:04 p.m. UTC | #3
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 Jan. 3, 2012, 6:08 p.m. UTC | #4
Or just using linux_binprm->filename with basename()
Alexander Graf Jan. 3, 2012, 6:28 p.m. UTC | #5
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
Alexander Graf Jan. 3, 2012, 6:29 p.m. UTC | #6
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
Fabio Erculiani Jan. 3, 2012, 6:46 p.m. UTC | #7
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 Jan. 3, 2012, 6:52 p.m. UTC | #8
Mumble,
that is what happens already...

Let me see why I get NULL here...
Alexander Graf Jan. 3, 2012, 6:52 p.m. UTC | #9
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
Fabio Erculiani Jan. 3, 2012, 6:54 p.m. UTC | #10
Yeah, debugging.

Moreover we have this scenario:

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

I guess we should use basename() anyway...?
Alexander Graf Jan. 3, 2012, 7:01 p.m. UTC | #11
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
Fabio Erculiani Jan. 3, 2012, 7:11 p.m. UTC | #12
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) ?
Alexander Graf Jan. 3, 2012, 7:12 p.m. UTC | #13
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
Fabio Erculiani Jan. 3, 2012, 7:21 p.m. UTC | #14
Done, it all works now ;-) !
diff mbox

Patch

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;
       }
     }