Review of attachment 273724 [details] [review]:

So looking at the current code, treating a char** as string of \0-separated fields looks questionable (at least). However dropping all but the first argument from the vector is clearly wrong.

::: src/shell-global.c
@@ +1163,3 @@
+  while (len < SIZE_MAX / 2) {
+    len *= 2;
+    if ((args2 = realloc (args, 2 * len)) == NULL)

Why multiplication by 2 (again)? Shouldn't the len parameter passed into sysctl() reflect the buffer size?

Looking at the man page[0], I wonder if something like

sysctl (mib, G_N_ELEMENTS (mib), NULL, &len, NULL, 0);
args = g_malloc0 (len + 1);
sysctl (min, G_N_ELEMENTS (mib), args, &len, NULL, 0);

wouldn't be a clearer approach (even with error handling added; considering that args is set to NULL initially, that's what the loop should be doing anyway now (overallocating aside), no?

I must admit that I'm not quite sure what the returned "len" would be in this case - the array size (e.g. argc) I suppose, and the actual arguments are callee owned?


[0] http://nixdoc.net/man-pages/OpenBSD/sysctl.3.html

@@ +1181,3 @@
+
+  arr = g_ptr_array_new ();
+  g_ptr_array_add (arr, buf);

Uhm - you are dropping all arguments past args[0]? How is that supposed to work with sth like "gnome-shell --mode=classic"?

> ::: src/shell-global.c
> @@ +1163,3 @@
> +  while (len < SIZE_MAX / 2) {
> +    len *= 2;
> +    if ((args2 = realloc (args, 2 * len)) == NULL)
> 
> Why multiplication by 2 (again)? Shouldn't the len parameter passed into
> sysctl() reflect the buffer size?

Yeah, that was an oversight from a previous patch. Sorry about that.

> Looking at the man page[0], I wonder if something like
> 
> sysctl (mib, G_N_ELEMENTS (mib), NULL, &len, NULL, 0);
> args = g_malloc0 (len + 1);
> sysctl (min, G_N_ELEMENTS (mib), args, &len, NULL, 0);
> 
> wouldn't be a clearer approach (even with error handling added; considering
> that args is set to NULL initially, that's what the loop should be doing anyway
> now (overallocating aside), no?

Oh you are absolutely right. This is much much simpler.

> +  arr = g_ptr_array_new ();
> +  g_ptr_array_add (arr, buf);
> 
> Uhm - you are dropping all arguments past args[0]? How is that supposed to work
> with sth like "gnome-shell --mode=classic"?

Well the gnome-shell process always appears as /usr/local/bin/gnome-shell whatever flags it was passed. Classic mode was tested successfully with this patch and the next one to come.

Thanks for the feedback Florian, it was much appreciated :-)

Created attachment 273763 [details] [review]
rewrite shell_global_reexec_self() for OpenBSD v2

(In reply to comment #2)
> > Uhm - you are dropping all arguments past args[0]? How is that supposed to work
> > with sth like "gnome-shell --mode=classic"?
> 
> Well the gnome-shell process always appears as /usr/local/bin/gnome-shell
> whatever flags it was passed. Classic mode was tested successfully with this
> patch and the next one to come.

How did you test classic mode? If started via GDM, then it is selected via the GNOME_SHELL_SESSION_MODE environment variable, so it's not an interesting test case this way. Does it really work if you start in normal mode, restart from a terminal using "gnome-shell --replace --mode=classic" and restart again via alt+f2 lg?

I would really expect something like

char **args_p;
for (args_p = args; *args_p; args_p++)
  g_ptr_array_add (arr, g_strdup (*args_p));

> How did you test classic mode? If started via GDM, then it is selected via the
> GNOME_SHELL_SESSION_MODE environment variable, so it's not an interesting test

Ooooooohhhhh :-)
Now that "unconfuses" me.

> case this way. Does it really work if you start in normal mode, restart from a
> terminal using "gnome-shell --replace --mode=classic" and restart again via
> alt+f2 lg?

Did you mean "alt+f2 r" ?

> I would really expect something like
> 
> char **args_p;
> for (args_p = args; *args_p; args_p++)
>   g_ptr_array_add (arr, g_strdup (*args_p));

Just tried it and "alt+f2 r" now restarts to normal mode all the time.

(In reply to comment #5)
> Did you mean "alt+f2 r" ?

Ugh, yeah - sorry.

(In reply to comment #6)
> (In reply to comment #5)
> > Did you mean "alt+f2 r" ?
> 
> Ugh, yeah - sorry.

Ok I modified the patch accordingly (not posted here) but the behavior is somewhat different now.
* login normally (gdm)
* /usr/local/bin/gnome-shell --replace --mode=classic
* alt+f2 r
* I am back in the regular mode (i.e. not classic)

As I mentioned earlier, ps(1) (which uses sysctl(3) as well) always list the gnome-shell process as: "/usr/local/bin/gnome-shell" (without any arguments).

Created attachment 273820 [details] [review]
rewrite shell_global_reexec_self() for OpenBSD v3

This is what I have so far.

Ok so it looks like my patch is correct but the underlying issue is indeed that gnome-shell does not register its arguments in the process list...
So gnome-shell --replace appears as gnome-shell as previously mentioned.
Not sure why the difference of behavior between Linux and OpenBSD here...

Hi Florian. Do you know how gnome-shell registers itself in the process list (is it special or something?).
Because whatever options I pass it, it always appears as 'gnome-shell' or '/usr/local/bin/gnome-shell' (depending on how I called it) -- but the args never appear. So I am wondering maybe there is some missing support somewhere which I need to look at...
Thank you.

(In reply to comment #10)
> Do you know how gnome-shell registers itself in the process list

It doesn't do anything to register itself; if it doesn't just work on BSD, then someone has to add it :-)

Created attachment 276169 [details] [review]
rewrite shell_global_reexec_self() for OpenBSD v4

Hi Florian. This patch now works fine in all situations (--mode=classic, ...).
Thanks.

Review of attachment 276169 [details] [review]:

Obviously broken, so no full review:

::: src/shell-global.c
@@ +1189,3 @@
   g_ptr_array_free (arr, TRUE);
+  if (args)
+    g_free (args);

This breaks on non-OpenBSD systems.

Created attachment 276172 [details] [review]
rewrite shell_global_reexec_self() for OpenBSD v5

Woops...

Review of attachment 276172 [details] [review]:

::: src/shell-global.c
@@ +1156,3 @@
+    return;
+
+  args = g_malloc0 (len);

From sysctl(3):
"The size of the available data can be determined by  calling sysctl() with
 a NULL parameter for oldp. The size of the available data will be returned
 in the location pointed to by oldlenp."

So len should correspond to argc, but later you use a NULL sentinel in the array, which you only actually allocate if len is bigger than the actual data (which is possible according to the man page, but not something to rely on). So unless I'm completely misreading, either your allocation is off-by-one, or you should use sth like for (i = 0; i < len; i++) later.

@@ +1190,3 @@
+#ifdef __OpenBSD__
+  if (args)
+    g_free (args);

Mmh, why the conditional here? Also:

"The information is copied into the buffer specified by oldp."

As I read this, you should use something like g_strfreev() here instead. (Incidentally, we apparently also leak on linux, so can you sneak in a
#ifdef __linux__
g_free (buf);
#elif defined __OpenBSD__
...?

(In reply to comment #15)
> Review of attachment 276172 [details] [review]:

Hi Florian.

Thanks for the new review (we'll get there ;-)).

> ::: src/shell-global.c
> @@ +1156,3 @@
> +    return;
> +
> +  args = g_malloc0 (len);
> 
> From sysctl(3):
> "The size of the available data can be determined by  calling sysctl() with
>  a NULL parameter for oldp. The size of the available data will be returned
>  in the location pointed to by oldlenp."
> 
> So len should correspond to argc, but later you use a NULL sentinel in the
> array, which you only actually allocate if len is bigger than the actual data
> (which is possible according to the man page, but not something to rely on). So
> unless I'm completely misreading, either your allocation is off-by-one, or you
> should use sth like for (i = 0; i < len; i++) later.

Ok I am not sure I am grasping exactly what you mean here...
Could you give me a small example?

> 
> @@ +1190,3 @@
> +#ifdef __OpenBSD__
> +  if (args)
> +    g_free (args);
> 
> Mmh, why the conditional here? Also:

Why? Because I am stupid, it was a left-over, I'll drop it.

> "The information is copied into the buffer specified by oldp."
> 
> As I read this, you should use something like g_strfreev() here instead.

g_strfreev will yeld to this:
in free(): error: bogus pointer 0x1099fab84020
Abort trap (core dumped)

Or maybe I should use g_strjoinv...

> (Incidentally, we apparently also leak on linux, so can you sneak in a
> #ifdef __linux__
> g_free (buf);
> #elif defined __OpenBSD__

Sure, will do.

(In reply to comment #16)
> (In reply to comment #15)
> > So unless I'm completely misreading, either your allocation is off-by-one, 
> > or you should use sth like for (i = 0; i < len; i++) later.
> 
> Ok I am not sure I am grasping exactly what you mean here...
> Could you give me a small example?

I mean that as I read the man page, "args" is the array ({ "foo", "bar" }) and "len" its length (2). But when filling the GPtrArray, your for loop expects a NULL-terminated array ({ "foo", "bar", NULL }) of length "len" + 1.


> > "The information is copied into the buffer specified by oldp."
> > 
> > As I read this, you should use something like g_strfreev() here instead.
> 
> g_strfreev will yeld to this:
> in free(): error: bogus pointer 0x1099fab84020
> Abort trap (core dumped)
> 
> Or maybe I should use g_strjoinv...

No, that sounds like the strings are not actually copied and g_free() is fine.

> I mean that as I read the man page, "args" is the array ({ "foo", "bar" }) and
> "len" its length (2). But when filling the GPtrArray, your for loop expects a
> NULL-terminated array ({ "foo", "bar", NULL }) of length "len" + 1.

Aaahhh, got you :-)
Ok actually, the "size of the available data" in this context means the size of the data type which in this case (KERN_PROC_ARGV) is ARG_MAX. And that is what is used for the allocation. I guess it is not very clear in the man page...

If that answers your question, I will cook a new patch with your other tweaks.
Thanks.

Created attachment 276919 [details] [review]
rewrite shell_global_reexec_self() for OpenBSD v6

Hi Florian. Latest iteration of the patch according to my comments and yours... hopefully this one will fly out.
Thank you.

Review of attachment 276919 [details] [review]:

Just one last nit - at the function top, we have:

void
shell_global_reexec_self (ShellGlobal *global)
{
  GPtrArray *arr;
  gsize len;
  char *buf;
  char *buf_p;
  char *buf_end;
  GError *error = NULL;

#if defined __linux__
All of buf, buf_p, buf_end and error are only used on Linux, so the compiler might complain about unused variables on OpenBSD - simply moving the #if defined up appropriately should fix that (no need for re-review, just do that before pushing)

Thanks Florian, all done and pushed as 3584887.