[PATCH 0/2] fix find_execable function and which code cleanup

Tito farmatito at tiscali.it
Thu May 1 18:51:24 UTC 2014


Hi,
while trying to cleanup the debianutils/which command I've spotted
a minor glitch in bb's find_execable function as it is not able to handle
zero lenght prefixes in the PATH variable:

http://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap08.html
8.3 Other Environment Variables
PATH
 snip
A zero-length prefix is a legacy feature that indicates the current 
working directory. It appears as two adjacent colons ( "::" ), as an 
initial colon preceding the rest of the list, or as a trailing colon 
following the rest of the list. A strictly conforming application shall 
use an actual pathname (such as .) to represent the current working 
directory in PATH .

The real which command (really a script) in debianutils 4.4 does in fact
handle  zero-length prefixes in PATH correctly:

  for ELEMENT in $PATH; do
    if [ -z "$ELEMENT" ]; then
     ELEMENT=.
    fi

So PATCH 1 actually fixes this, the size impact is huge
and therefore it is dependent on CONFIG_DEKTOP:

find_execable                                         81     182    +101

as parsing the PATH was not trivial as once you've started
it is hard to tell if a delimiter was a leading, trailing or double
colon. 
I've tried and retried but no solution was satisfactory
as always some corner case  was not parsed correctly,
so I changed strategy and normalize the PATH variable
so that it is easier to parse before starting the parsing.
Hints about a better solution with less code are welcome!!!

--- libbb/execable.c.orig       2013-06-02 13:56:34.000000000 +0200
+++ libbb/execable.c    2014-05-01 20:30:02.966512357 +0200
@@ -30,9 +30,37 @@
  */
 char* FAST_FUNC find_execable(const char *filename, char **PATHp)
 {
+       /* http://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap08.html
+        * 8.3 Other Environment Variables - PATH
+        * A zero-length prefix is a legacy feature that indicates the current
+        * working directory. It appears as two adjacent colons ( "::" ), as an
+        * initial colon preceding the rest of the list, or as a trailing colon
+        * following the rest of the list.
+        */
+
+       IF_DESKTOP(int len;)
        char *p, *n;
 
        p = *PATHp;
+#if ENABLE_DESKTOP
+       /* Normalize PATH and add . for cwd where needed */
+       while ((n = strstr(p, "::")) != NULL) { /* Double :: */
+               *n = '\0';
+               n += 2;
+               p = xasprintf("%s:.:%s", p, n);
+       }
+
+       if (*p == ':' && p[1]  != '.') {  /* Leading single : */
+               p = xasprintf(".%s", p);
+       }
+
+       len = strlen(p) - 1;
+       if (p[len] == ':' &&  p[len - 1] != '.') { /* Trailing single : */
+               p = xasprintf("%s.:", p);
+       }
+
+       *PATHp = p;
+#endif
        while (p) {
                n = strchr(p, ':');
                if (n)


The find execable_function and the exists_execable function
are used only in which and ifupdown and this change
seems not to affect them negatively.

PATCH 2 removes the #if ENABLE_DESKTOP part from
which relative to the -a option and unifies the code.
The size increase is negligible vs the version with
ENABLE_DESKTOP disabled:

scripts/bloat-o-meter busybox.old.nodesktop busybox_unstripped
function                                             old     new   delta
which_main                                           193     202      +9
------------------------------------------------------------------------------
(add/remove: 0/0 grow/shrink: 1/0 up/down: 9/0)                 Total: 9 bytes

So it makes sense to trade 9 bytes for the improved functionality
and reduced number of code lines.

In the end our which command looks like this:

int which_main(int argc UNUSED_PARAM, char **argv)
{
	int found;
	char *path;
	char *p;
	char *tmp;
	int status = 0;

	opt_complementary = "-1";	/* at least one argument */
	getopt32(argv, "a");
	argv += optind;
	
	do {
		found = 0;
		   /* If file contains a slash don't use PATH */
		if (strchr(*argv, '/') && execable_file(*argv)) {
			/* returns non-negative number on success, or EOF (-1) on error */
			found = puts(*argv);
		} else {
			p = getenv("PATH");
			if (p == NULL) {
				p =  (char *)bb_default_root_path;
			}
			path = tmp = xstrdup(p);
			while ((p = find_execable(*argv, &tmp)) != NULL) {
				found = puts(p);
				free(p);
				if (!option_mask32) /* -a not set*/
					break;
			}
			free(path);
		}
		status |= (!found);
	} while (*(++argv) != NULL);

	fflush_stdout_and_exit(status);
}

Hints, critics and improvements are welcome,

Ciao,
Tito

P.S.: the patches could be applied indipendently.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 01_execable.patch
Type: text/x-patch
Size: 1419 bytes
Desc: not available
URL: <http://lists.busybox.net/pipermail/busybox/attachments/20140501/2079dab1/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 02_which.patch
Type: text/x-patch
Size: 2468 bytes
Desc: not available
URL: <http://lists.busybox.net/pipermail/busybox/attachments/20140501/2079dab1/attachment-0001.bin>


More information about the busybox mailing list