[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openbsd-tech
Subject:    Re: fortune: allow to use symlinks
From:       Vadim Zhukov <persgray () gmail ! com>
Date:       2020-12-15 12:10:05
Message-ID: 1d0d91074182c8b4 () persx240 ! my ! domain
[Download RAW message or body]

> I very rarely use fortune and I'm not familiar with the codebase, but
> from some quick testing and code-scanning I found the following:
> is_fortfile appends .dat to the input file and sees if it's accessible.
> Adding a symlink to the corresponding .dat file make the thing work
> perfectly well for me. Your diff seems to work, because you resolve
> the path, which in turn gives back the correct location for the .dat,
> not because symlinks are not suported.

Thank you very much for review. Indeed, adding second symlink fixes
the initial issue, so the patch now seems useless...

> Personally I don't see good reason to add the additional logic which
> can be solved with a second simple symlink, but if others think it's
> worth it, comments inline.

... but following your comments I've discovered that copy() function
in fortune.c can return NULL, and that value isn't checked.

So I'm posting the whole diff for the sake of completeness, and what
I really think worths committing is the last chunk, replacing "return
NULL" with "err(1, NULL)" in copy(). Okay for it?

> martijn@
> 
> On Tue, 2020-12-15 at 01:09 +0300, Vadim Zhukov wrote:
> > Hello, all.
> > 
> > This allows fortune(6) to open fortune files via symlinks.
> > Maybe this is a stupid idea, I dunno, but it seems inconsistent
> > that I can't put a symlink to my fortunes collection in
> > /usr/share/games/fortune and then simply call "fortune foo"
> > instead of "fortune /usr/local/share/games/fortune/ru/foo".
> > 
> > I decided to call readlink(2) explicitly rather than adding
> > check of file type being open, since that resulted in less code.
> > 
> > Ah, and yes, I have a port of Russian fortune files pending,
> > but that's for another list and another day.
> > 
> > So... okay/notokay anyone? :)

--
WBR,
  Vadim Zhukov


Index: fortune.c
===================================================================
RCS file: /cvs/src/games/fortune/fortune/fortune.c,v
retrieving revision 1.60
diff -u -p -r1.60 fortune.c
--- fortune.c	10 Aug 2017 17:00:08 -0000	1.60
+++ fortune.c	15 Dec 2020 11:46:00 -0000
@@ -39,6 +39,7 @@
 #include <ctype.h>
 #include <dirent.h>
 #include <err.h>
+#include <errno.h>
 #include <fcntl.h>
 #include <limits.h>
 #include <locale.h>
@@ -388,8 +389,9 @@ add_file(int percent, char *file, char *
     FILEDESC *parent)
 {
 	FILEDESC	*fp;
+	ssize_t		 lnklen;
 	int		fd;
-	char		*path, *offensive;
+	char		*path, *offensive, lnkpath[PATH_MAX];
 	bool		was_malloc;
 	bool		isdir;
 
@@ -420,8 +422,22 @@ add_file(int percent, char *file, char *
 	}
 
 	DPRINTF(1, (stderr, "adding file \"%s\"\n", path));
+
 over:
+	lnklen = readlink(path, lnkpath, sizeof(lnkpath));
+	if (lnklen == -1) {
+		if (errno != EINVAL)
+			goto openfailed;
+	} else {
+		lnkpath[lnklen] = '\0';
+		if (was_malloc)
+			free(path);
+		path = copy(lnkpath, NULL);
+		was_malloc = 1;
+	}
+
 	if ((fd = open(path, O_RDONLY)) < 0) {
+openfailed:
 		/*
 		 * This is a sneak.  If the user said -a, and if the
 		 * file we're given isn't a file, we check to see if
@@ -718,7 +734,7 @@ copy(char *str, char *suf)
 	char	*new;
 
 	if (asprintf(&new, "%s%s", str, suf ? suf : "") == -1)
-		return NULL;
+		err(1, NULL);
 	return new;
 }
 

[prev in list] [next in list] [prev in thread] [next in thread]