After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 728280 - platform_get_argv0: fix sysctl(3) use on OpenBSD
platform_get_argv0: fix sysctl(3) use on OpenBSD
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: general
2.40.x
Other OpenBSD
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2014-04-15 17:57 UTC by Antoine Jacoutot
Modified: 2014-04-20 21:17 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
fix sysctl(3) use on OpenBSD (1.98 KB, patch)
2014-04-15 17:57 UTC, Antoine Jacoutot
needs-work Details | Review
clean up platform_get_argv0 (OpenBSD) (991 bytes, patch)
2014-04-16 05:52 UTC, Antoine Jacoutot
none Details | Review
clean up platform_get_argv0 (OpenBSD) v2 (1.28 KB, patch)
2014-04-16 08:29 UTC, Antoine Jacoutot
none Details | Review

Description Antoine Jacoutot 2014-04-15 17:57:48 UTC
Created attachment 274386 [details] [review]
 fix sysctl(3) use on OpenBSD

Hi.

This patch rework platform_get_argv0 to properly use sysctl(3) on OpenBSD and add some sanity checks.

Regression tests are happy:
./option-argv0 --verbose --debug-log
GTest: random seed: R02Sb5d798ac22b0eceee4e4242d53855b53
{*LOG(binary):LOG*}
{*LOG(start suite):{}:LOG*}
{*LOG(start suite):{option}:LOG*}
{*LOG(start):{/option/argv0}:LOG*}
GTest: run: /option/argv0
GTest: result: OK
{*LOG(stop):{/option/argv0}:(0;0;0.000962):LOG*}
{*LOG(stop suite):{option}:LOG*}
{*LOG(stop suite):{}:LOG*}
Comment 1 Allison Karlitskaya (desrt) 2014-04-15 22:30:44 UTC
Review of attachment 274386 [details] [review]:

::: glib/goption.c
@@ +1770,3 @@
   int mib[] = { CTL_KERN, KERN_PROC_ARGS, getpid(), KERN_PROC_ARGV };
+  args = NULL;
+  len = 128;

What is the purpose of initialising this?  Is it an in/out for the next call?  If so, 128 seems a bit small...

@@ +1777,1 @@
+  args = g_malloc0 (len + 1);

I'm fairly sure this must be incorrect somehow.

What is 'len' as returned by the syscall?  args is a 'char **'.  Is this the number of 'char *' in the array or is it the byte length of the array?

Probably makes more sense to use g_new() here... or if 'len' really is the byte length then you need to add sizeof(char*) for the NULL -- not 1.  If 'len' is a bytesize rather then a length, the name 'size' would be nicer.

@@ +1777,2 @@
+  args = g_malloc0 (len + 1);
+  if (args == NULL)

drop this check please -- g_malloc() never fails.

@@ +1783,3 @@
+
+  buf[0] = '\0';
+  for (argv = args; *argv != NULL; argv++) {

This is much easier written g_strjoinv(" ", ...);

But if the goal is to extract only the basename of argv0 then why do you paste together all of the arguments as well?

@@ +1790,3 @@
+
+  g_free (args);
+  cmdline = g_strdup_printf ("%s", buf);

better written g_strdup(buf);

... but there is little point in allocating this string temporarily in order to call basename() on it and free it just below.  You could just call basename() on buf directly.
Comment 2 Antoine Jacoutot 2014-04-16 05:52:37 UTC
Created attachment 274417 [details] [review]
clean up platform_get_argv0 (OpenBSD)

Hi Ryan.

Sorry I was reviewing a lot of similar code and I totally missed the point for that part... my copy/paste was too savage ;-)
Anyway, since I am around, this small patch cleans up unneeded headers and use g_realloc instead of realloc.
Comment 3 Antoine Jacoutot 2014-04-16 08:29:30 UTC
Created attachment 274427 [details] [review]
clean up platform_get_argv0 (OpenBSD) v2
Comment 4 Antoine Jacoutot 2014-04-19 10:12:31 UTC
Hi Ryan. Any other input on that last diff?
Thanks :-)
Comment 5 Allison Karlitskaya (desrt) 2014-04-20 19:29:22 UTC
Nope.  Looks like an improvement -- please go ahead.
Comment 6 Antoine Jacoutot 2014-04-20 21:17:53 UTC
Thanks Ryan. Pushed as 9352cdb.