readlink bugs

[Bernhard Fischer]

On Fri, Oct 20, 2006 at 10:13:31AM +0200, Bernhard Fischer wrote:
>On Thu, Oct 19, 2006 at 07:30:56PM +0200, Denis Vlasenko wrote:
>>On Wednesday 18 October 2006 07:41, Alexander Griesser wrote:
>
>>Bingo! static busybox doesn't fflush stdout!
>>This workaround made readlink work:
>
>No! :)
>
>>int readlink_main(int argc, char **argv)
>>{
>>        char *buf;
>>        unsigned opt = ENABLE_FEATURE_READLINK_FOLLOW ?
>>                        getopt32(argc, argv, "f") : 0;
>>
>>        if (argc != (ENABLE_FEATURE_READLINK_FOLLOW ? optind + 1 : 2))
>>                        bb_show_usage();
>>
>>        if (opt & READLINK_FLAG_f)
>>                buf = realpath(argv[optind], bb_common_bufsiz1);
>>        else
>>                buf = xreadlink(argv[ENABLE_FEATURE_READLINK_FOLLOW ? optind : 1]);
>>
>>        if (!buf)
>>                return EXIT_FAILURE;
>>        puts(buf);
>->+       fflush(stdout);
>>
>>        if (ENABLE_FEATURE_CLEAN_UP && buf != bb_common_bufsiz1)
>>                free(buf);
>>
>->        return EXIT_SUCCESS;
>+	bb_fflush_stdout_and_exit(EXIT_SUCCESS);
>>}
>>
>>Need to take a look how glibc managed to botch it...
>
>Please don't botch busybox but reuse the stuff that is already there ;)
>   text    data     bss     dec     hex filename
>     98       0       0      98      62 debianutils/readlink.o.orig
>    110       0       0     110      6e debianutils/readlink.o.vda
>    103       0       0     103      67 debianutils/readlink.o.bb_fflush
>
>cheers,

hm. Please see the questions in the attached patch that results in
shrinking it back to under 100 bytes.

   text    data     bss     dec     hex filename
     94       0       0      94      5e debianutils/readlink.o.attached

Attached patchlet also contains an extension to getopt32's
opt_complementary; =N to check for exactly N arguments.
Rational: Several applets do something to the effect of
(if argc != mynumber)
	bb_show_usage();
While it seems that saying opt_complementary="-1?1" for mynumber=1
works, it doubles the size as compared to the proposed "=1" string.
Needless to say that the impl of that =N support in getopt32.c that i
have attached is surprisingly big, but i didn't look closely on how to
keep it down to a reasonable size increase:
$ size libbb/getopt32.o*
   text    data     bss     dec     hex filename
    979       4       0     983     3d7 libbb/getopt32.o.oorig
   1014       4       0    1018     3fa libbb/getopt32.o

Is this =N support something that you consider useful?


A few observations about the current readlink (bugs):
$ rm -f no ../no ; touch yep
$ for i in '' ./busybox;do $i readlink no;echo ret=$?;done
ret=1
readlink: no: No such file or directory
ret=1
$ emits wrong perror (see comment in the patch)


$ for i in '' ./busybox;do $i readlink yep;echo ret=$?;done
ret=1
readlink: yep: Invalid argument
ret=1
$ again, emits perror wrongly


$ for i in '' ./busybox;do $i readlink -f no;echo ret=$?;done
/tmp/busybox/no
ret=0
ret=1
$ does not emit nor normalize fname (see comment)


$ for i in '' ./busybox;do $i readlink -f ../no;echo ret=$?;done
/tmp/no
ret=0
ret=1
$ same as for ./no

-------------- next part --------------
Index: debianutils/readlink.c
===================================================================
--- debianutils/readlink.c	(revision 16410)
+++ debianutils/readlink.c	(working copy)
@@ -18,16 +18,35 @@
 int readlink_main(int argc, char **argv)
 {
 	char *buf;
-	unsigned opt = ENABLE_FEATURE_READLINK_FOLLOW ?
+	unsigned opt;
+	char *fname;
+
+	/* We need exactly one non-option argument.  */
+	USE_FEATURE_READLINK_FOLLOW(opt_complementary = "-1?1";)
+	opt = ENABLE_FEATURE_READLINK_FOLLOW ?
 			getopt32(argc, argv, "f") : 0;
 
-	if (argc != (ENABLE_FEATURE_READLINK_FOLLOW ? optind + 1 : 2))
+	/* Regardless of follow support, opt is now 0 if -f was not seen,
+	 * and 1 otherwise.  */
+	SKIP_FEATURE_READLINK_FOLLOW(if (argc !=  opt + 2)
 			bb_show_usage();
+	)
+	/* Caching the array deref saved one byte for me and was optimized away
+	 * if follow support is turned off.  */
+	fname = argv[opt+1];
 
-	if (opt & READLINK_FLAG_f)
-		buf = realpath(argv[optind], bb_common_bufsiz1);
-	else
-		buf = xreadlink(argv[ENABLE_FEATURE_READLINK_FOLLOW ? optind : 1]);
+	if (opt & READLINK_FLAG_f) {
+		buf = realpath(fname, bb_common_bufsiz1);
+		/* If it is not a file, then we have to emit "$CWD/fname".  */
+/* XXX: we'd need to normalize relative filenames WRT CWD here..
+ * -f ../nosuchfile becomes simplify_path(getcwd() ## fname
+ */
+	} else {
+/* XXX: Funnily, this a) calls bb_perror_msg which we do not want and
+ * b) exits with a failure which we want neither. Sad but we apparently
+ * have to spin our own version here, no? */
+		buf = xreadlink(fname);
+	}
 
 	if (!buf)
 		return EXIT_FAILURE;
@@ -36,5 +55,5 @@ int readlink_main(int argc, char **argv)
 	if (ENABLE_FEATURE_CLEAN_UP && buf != bb_common_bufsiz1)
 		free(buf);
 
-	return EXIT_SUCCESS;
+	bb_fflush_stdout_and_exit(EXIT_SUCCESS);
 }
Index: libbb/getopt32.c
===================================================================
--- libbb/getopt32.c	(revision 16410)
+++ libbb/getopt32.c	(working copy)
@@ -188,6 +188,10 @@ Special characters:
         by a single digit (0-9) means that at least N non-option
         arguments must be present on the command line
 
+ "=N"   An equal sign as the first char in a opt_complementary group followed
+        by a single digit (0-9) means that exactly N non-option
+        arguments must be present on the command line
+
  "V-"   An option with dash before colon or end-of-line results in
         bb_show_usage being called if this option is encountered.
         This is typically used to implement "print verbose usage message
@@ -400,6 +404,11 @@ getopt32(int argc, char **argv, const ch
 			}
 			continue;
 		}
+		if (*s == '=') {
+			min_arg = max_arg = c - '0';
+			s++;
+			continue;
+		}
 		for (on_off = complementary; on_off->opt; on_off++)
 			if (on_off->opt == *s)
 				break;