On Tue, 2020-12-15 at 15:10 +0300, Vadim Zhukov wrote:
> > 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?
OK martijn@ for this last bit.
>
> > 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;
> }
>
