[PATCH] eal: remove unnecessary argv[0] handling

[PATCH] eal: remove unnecessary argv[0] handling

[Stephen Hemminger]

The rte_eal_init function looks at argv[0] to determine
the program name to pass to the log init function.
This is both unnecessary and in a corner case a problem.

Parsing argv[0] is unnecessary because the function openlog()
already determines the log identifier from program name if
NULL is passed, see openlog man page:
    The string pointed to by ident is prepended to every message, and is
    typically set to the program name. If ident is NULL, the program name
    is used.  (POSIX.1-2008 does not specify the behavior when ident is
    NULL.)

It is possible to run a program with argc=0 and argv[0]=NULL.
This would cause rte_eal_init() to dereference a null pointer.
Not a really useful feature, but better for DPDK get a SEGV
if it doesn't need to.

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 lib/eal/linux/eal.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/lib/eal/linux/eal.c b/lib/eal/linux/eal.c
index 60b49248388e..6bc7cdf440b8 100644
--- a/lib/eal/linux/eal.c
+++ b/lib/eal/linux/eal.c
@@ -966,8 +966,6 @@ rte_eal_init(int argc, char **argv)
 	pthread_t thread_id;
 	static uint32_t run_once;
 	uint32_t has_run = 0;
-	const char *p;
-	static char logid[PATH_MAX];
 	char cpuset[RTE_CPU_AFFINITY_STR_LEN];
 	char thread_name[RTE_MAX_THREAD_NAME_LEN];
 	bool phys_addrs;
@@ -989,8 +987,6 @@ rte_eal_init(int argc, char **argv)
 		return -1;
 	}
 
-	p = strrchr(argv[0], '/');
-	strlcpy(logid, p ? p + 1 : argv[0], sizeof(logid));
 	thread_id = pthread_self();
 
 	eal_reset_internal_config(internal_conf);
@@ -1165,7 +1161,7 @@ rte_eal_init(int argc, char **argv)
 #endif
 	}
 
-	if (eal_log_init(logid, internal_conf->syslog_facility) < 0) {
+	if (eal_log_init(NULL, internal_conf->syslog_facility) < 0) {
 		rte_eal_init_alert("Cannot init logging.");
 		rte_errno = ENOMEM;
 		__atomic_store_n(&run_once, 0, __ATOMIC_RELAXED);
-- 
2.34.1

Re: [PATCH] eal: remove unnecessary argv[0] handling

[Thomas Monjalon]

02/02/2022 20:47, Stephen Hemminger:
> The rte_eal_init function looks at argv[0] to determine
> the program name to pass to the log init function.
> This is both unnecessary and in a corner case a problem.
> 
> Parsing argv[0] is unnecessary because the function openlog()
> already determines the log identifier from program name if
> NULL is passed, see openlog man page:
>     The string pointed to by ident is prepended to every message, and is
>     typically set to the program name. If ident is NULL, the program name
>     is used.  (POSIX.1-2008 does not specify the behavior when ident is
>     NULL.)

What about POSIX warning?
Did you test it? Which libc? What about musl?

[...]
> -	if (eal_log_init(logid, internal_conf->syslog_facility) < 0) {
> +	if (eal_log_init(NULL, internal_conf->syslog_facility) < 0) {

We could completely remove the logid parameter.

Re: [PATCH] eal: remove unnecessary argv[0] handling

[Stephen Hemminger]

On Wed, 09 Feb 2022 15:58:26 +0100
Thomas Monjalon <thomas@monjalon.net> wrote:

> 02/02/2022 20:47, Stephen Hemminger:
> > The rte_eal_init function looks at argv[0] to determine
> > the program name to pass to the log init function.
> > This is both unnecessary and in a corner case a problem.
> > 
> > Parsing argv[0] is unnecessary because the function openlog()
> > already determines the log identifier from program name if
> > NULL is passed, see openlog man page:
> >     The string pointed to by ident is prepended to every message, and is
> >     typically set to the program name. If ident is NULL, the program name
> >     is used.  (POSIX.1-2008 does not specify the behavior when ident is
> >     NULL.)  
> 
> What about POSIX warning?

There are many cases where it is practical and useful to do non-POSIX
things (see _GNU_SOURCE).  Look at pthread_setaffinity_np() etc.

> Did you test it? Which libc? What about musl?

No, I don't have or use alternative libc versions.

> 
> [...]
> > -	if (eal_log_init(logid, internal_conf->syslog_facility) < 0) {
> > +	if (eal_log_init(NULL, internal_conf->syslog_facility) < 0) {  
> 
> We could completely remove the logid parameter.

Yes, it could go away.

Re: [PATCH] eal: remove unnecessary argv[0] handling

[Stephen Hemminger]

On Wed, 09 Feb 2022 15:58:26 +0100
Thomas Monjalon <thomas@monjalon.net> wrote:

> 02/02/2022 20:47, Stephen Hemminger:
> > The rte_eal_init function looks at argv[0] to determine
> > the program name to pass to the log init function.
> > This is both unnecessary and in a corner case a problem.
> > 
> > Parsing argv[0] is unnecessary because the function openlog()
> > already determines the log identifier from program name if
> > NULL is passed, see openlog man page:
> >     The string pointed to by ident is prepended to every message, and is
> >     typically set to the program name. If ident is NULL, the program name
> >     is used.  (POSIX.1-2008 does not specify the behavior when ident is
> >     NULL.)  
> 
> What about POSIX warning?
> Did you test it? Which libc? What about musl?

Source for musl shows that openlog accepts NULL as ident.
It then generates log messages with ident in the message.

Re: [PATCH] eal: remove unnecessary argv[0] handling

[Thomas Monjalon]

09/02/2022 16:45, Stephen Hemminger:
> On Wed, 09 Feb 2022 15:58:26 +0100
> Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> > 02/02/2022 20:47, Stephen Hemminger:
> > > The rte_eal_init function looks at argv[0] to determine
> > > the program name to pass to the log init function.
> > > This is both unnecessary and in a corner case a problem.
> > > 
> > > Parsing argv[0] is unnecessary because the function openlog()
> > > already determines the log identifier from program name if
> > > NULL is passed, see openlog man page:
> > >     The string pointed to by ident is prepended to every message, and is
> > >     typically set to the program name. If ident is NULL, the program name
> > >     is used.  (POSIX.1-2008 does not specify the behavior when ident is
> > >     NULL.)  
> > 
> > What about POSIX warning?
> > Did you test it? Which libc? What about musl?
> 
> Source for musl shows that openlog accepts NULL as ident.
> It then generates log messages with ident in the message.

In this case, it will be empty string.
It seems this solution does not work with musl.

Re: [PATCH] eal: remove unnecessary argv[0] handling

[Stephen Hemminger]

On Wed, 09 Feb 2022 19:51:15 +0100
Thomas Monjalon <thomas@monjalon.net> wrote:

> 09/02/2022 16:45, Stephen Hemminger:
> > On Wed, 09 Feb 2022 15:58:26 +0100
> > Thomas Monjalon <thomas@monjalon.net> wrote:
> >   
> > > 02/02/2022 20:47, Stephen Hemminger:  
> > > > The rte_eal_init function looks at argv[0] to determine
> > > > the program name to pass to the log init function.
> > > > This is both unnecessary and in a corner case a problem.
> > > > 
> > > > Parsing argv[0] is unnecessary because the function openlog()
> > > > already determines the log identifier from program name if
> > > > NULL is passed, see openlog man page:
> > > >     The string pointed to by ident is prepended to every message, and is
> > > >     typically set to the program name. If ident is NULL, the program name
> > > >     is used.  (POSIX.1-2008 does not specify the behavior when ident is
> > > >     NULL.)    
> > > 
> > > What about POSIX warning?
> > > Did you test it? Which libc? What about musl?  
> > 
> > Source for musl shows that openlog accepts NULL as ident.
> > It then generates log messages with ident in the message.  
> 
> In this case, it will be empty string.
> It seems this solution does not work with musl.

Good news: musl is available as debian package
Bad news: DPDK build is broken with musl


Library IPSec_MB found: YES

drivers/crypto/ipsec_mb/meson.build:19:4: ERROR: Could not get define 'IMB_VERSION_STR'

Re: [PATCH] eal: remove unnecessary argv[0] handling

[Thomas Monjalon]

09/02/2022 20:29, Stephen Hemminger:
> On Wed, 09 Feb 2022 19:51:15 +0100
> Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> > 09/02/2022 16:45, Stephen Hemminger:
> > > On Wed, 09 Feb 2022 15:58:26 +0100
> > > Thomas Monjalon <thomas@monjalon.net> wrote:
> > >   
> > > > 02/02/2022 20:47, Stephen Hemminger:  
> > > > > The rte_eal_init function looks at argv[0] to determine
> > > > > the program name to pass to the log init function.
> > > > > This is both unnecessary and in a corner case a problem.
> > > > > 
> > > > > Parsing argv[0] is unnecessary because the function openlog()
> > > > > already determines the log identifier from program name if
> > > > > NULL is passed, see openlog man page:
> > > > >     The string pointed to by ident is prepended to every message, and is
> > > > >     typically set to the program name. If ident is NULL, the program name
> > > > >     is used.  (POSIX.1-2008 does not specify the behavior when ident is
> > > > >     NULL.)    
> > > > 
> > > > What about POSIX warning?
> > > > Did you test it? Which libc? What about musl?  
> > > 
> > > Source for musl shows that openlog accepts NULL as ident.
> > > It then generates log messages with ident in the message.  
> > 
> > In this case, it will be empty string.
> > It seems this solution does not work with musl.
> 
> Good news: musl is available as debian package
> Bad news: DPDK build is broken with musl
> 
> 
> Library IPSec_MB found: YES
> 
> drivers/crypto/ipsec_mb/meson.build:19:4: ERROR: Could not get define 'IMB_VERSION_STR'

This error is not related to musl.

Compilation on Alpine (with musl) is tested regularly,
it works for me.

[PATCH v2] Subject: eal: remove unnecessary argv[0] handling

[Stephen Hemminger]

The rte_eal_init function only evalutes argv[0] to determine
the program name to pass to the log init function.
This is both unnecessary and in a corner case a problem.

It is possible to run a program with argc=0 and argv[0]=NULL.
This would cause rte_eal_init() to dereference a null pointer.
Not a really useful feature, but better for DPDK get a SEGV
if it doesn't need to.

Parsing argv[0] is unnecessary because the function openlog()
already determines the log identifier from program name if
NULL is passed.

openlog man page:
    The string pointed to by ident is prepended to every message, and is
    typically set to the program name. If ident is NULL, the program name
    is used.  (POSIX.1-2008 does not specify the behavior when ident is
    NULL.)

Since eal_log_init() is internal just drop the unused argument.

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 lib/eal/common/eal_log.h  | 2 +-
 lib/eal/linux/eal.c       | 6 +-----
 lib/eal/linux/eal_log.c   | 4 ++--
 lib/eal/windows/eal.c     | 2 +-
 lib/eal/windows/eal_log.c | 2 +-
 5 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/lib/eal/common/eal_log.h b/lib/eal/common/eal_log.h
index c784fa604389..d0282b3258da 100644
--- a/lib/eal/common/eal_log.h
+++ b/lib/eal/common/eal_log.h
@@ -11,7 +11,7 @@
 /*
  * Initialize the default log stream.
  */
-int eal_log_init(const char *id, int facility);
+int eal_log_init(int facility);
 
 /*
  * Determine where log data is written when no call to rte_openlog_stream.
diff --git a/lib/eal/linux/eal.c b/lib/eal/linux/eal.c
index 9c8395ab14d0..a6e8c5793334 100644
--- a/lib/eal/linux/eal.c
+++ b/lib/eal/linux/eal.c
@@ -966,8 +966,6 @@ rte_eal_init(int argc, char **argv)
 	pthread_t thread_id;
 	static uint32_t run_once;
 	uint32_t has_run = 0;
-	const char *p;
-	static char logid[PATH_MAX];
 	char cpuset[RTE_CPU_AFFINITY_STR_LEN];
 	char thread_name[RTE_MAX_THREAD_NAME_LEN];
 	bool phys_addrs;
@@ -989,8 +987,6 @@ rte_eal_init(int argc, char **argv)
 		return -1;
 	}
 
-	p = strrchr(argv[0], '/');
-	strlcpy(logid, p ? p + 1 : argv[0], sizeof(logid));
 	thread_id = pthread_self();
 
 	eal_reset_internal_config(internal_conf);
@@ -1165,7 +1161,7 @@ rte_eal_init(int argc, char **argv)
 #endif
 	}
 
-	if (eal_log_init(logid, internal_conf->syslog_facility) < 0) {
+	if (eal_log_init(internal_conf->syslog_facility) < 0) {
 		rte_eal_init_alert("Cannot init logging.");
 		rte_errno = ENOMEM;
 		__atomic_store_n(&run_once, 0, __ATOMIC_RELAXED);
diff --git a/lib/eal/linux/eal_log.c b/lib/eal/linux/eal_log.c
index c0aa1007c4df..e5cc10737b37 100644
--- a/lib/eal/linux/eal_log.c
+++ b/lib/eal/linux/eal_log.c
@@ -46,7 +46,7 @@ static cookie_io_functions_t console_log_func = {
  * once memzones are available.
  */
 int
-eal_log_init(const char *id, int facility)
+eal_log_init(int facility)
 {
 	FILE *log_stream;
 
@@ -54,7 +54,7 @@ eal_log_init(const char *id, int facility)
 	if (log_stream == NULL)
 		return -1;
 
-	openlog(id, LOG_NDELAY | LOG_PID, facility);
+	openlog(NULL, LOG_NDELAY | LOG_PID, facility);
 
 	eal_log_set_default(log_stream);
 
diff --git a/lib/eal/windows/eal.c b/lib/eal/windows/eal.c
index ca3c41aaa7f1..3133a934dd3c 100644
--- a/lib/eal/windows/eal.c
+++ b/lib/eal/windows/eal.c
@@ -280,7 +280,7 @@ rte_eal_init(int argc, char **argv)
 	enum rte_iova_mode iova_mode;
 	int ret;
 
-	eal_log_init(NULL, 0);
+	eal_log_init(0);
 
 	eal_log_level_parse(argc, argv);
 
diff --git a/lib/eal/windows/eal_log.c b/lib/eal/windows/eal_log.c
index d4ea47f1c824..df995c337998 100644
--- a/lib/eal/windows/eal_log.c
+++ b/lib/eal/windows/eal_log.c
@@ -8,7 +8,7 @@
 
 /* set the log to default function, called during eal init process. */
 int
-eal_log_init(__rte_unused const char *id, __rte_unused int facility)
+eal_log_init(__rte_unused int facility)
 {
 	rte_openlog_stream(stderr);
 
-- 
2.34.1

[PATCH v3] eal: simplify argv[0] handling

[Stephen Hemminger]

The rte_eal_init function looks at argv[0] to determine
the program name to pass to the log init function.
But in corner cases argv[0] maybe NULL leading to a SEGV.

The code here is just using argv[0] to generate logid which
does not have to be in a static string, openlog() will handle
a const char pointer.

Simple workaround for argv[0] being NULL is to pass NULL
to openlog() and let it handle it. Both glibc, and musl
handle this case.

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---

v3 - redo to make this limited to just the null argv[0] bug

 lib/eal/linux/eal.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/lib/eal/linux/eal.c b/lib/eal/linux/eal.c
index 9c8395ab14d0..c0ff325c4ce9 100644
--- a/lib/eal/linux/eal.c
+++ b/lib/eal/linux/eal.c
@@ -966,8 +966,7 @@ rte_eal_init(int argc, char **argv)
 	pthread_t thread_id;
 	static uint32_t run_once;
 	uint32_t has_run = 0;
-	const char *p;
-	static char logid[PATH_MAX];
+	const char *logid = NULL;
 	char cpuset[RTE_CPU_AFFINITY_STR_LEN];
 	char thread_name[RTE_MAX_THREAD_NAME_LEN];
 	bool phys_addrs;
@@ -989,8 +988,12 @@ rte_eal_init(int argc, char **argv)
 		return -1;
 	}
 
-	p = strrchr(argv[0], '/');
-	strlcpy(logid, p ? p + 1 : argv[0], sizeof(logid));
+	if (argv && argv[0]) {
+		const char *p = strrchr(argv[0], '/');
+
+		logid = p ? p + 1 : argv[0];
+	}
+
 	thread_id = pthread_self();
 
 	eal_reset_internal_config(internal_conf);
-- 
2.34.1