Re: fortune: allow to use symlinks
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 - 1.60
> +++ fortune.c 15 Dec 2020 11:46:00 -
> @@ -39,6 +39,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -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];
> boolwas_malloc;
> boolisdir;
>
> @@ -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;
> }
>
Re: fortune: allow to use symlinks
> 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 - 1.60
+++ fortune.c 15 Dec 2020 11:46:00 -
@@ -39,6 +39,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -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];
boolwas_malloc;
boolisdir;
@@ -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;
}
Re: fortune: allow to use symlinks
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.
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.
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 - 1.60
> +++ fortune.c 14 Dec 2020 22:05:33 -
> @@ -39,6 +39,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -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];
> boolwas_malloc;
> boolisdir;
>
> @@ -420,8 +422,22 @@ add_file(int percent, char *file, char *
> }
>
> DPRINTF(1, (stderr, "adding file \"%s\"\n", path));
> +
> over:
> + lnklen = readlink(path, lnkpath, PATH_MAX);
I'd prefer sizeof(lnkpath) instead of PATH_MAX, just to be a little more
robust.
> + if (lnklen == -1) {
> + if (errno != EINVAL)
> + goto openfailed;
> + } else {
> + lnkpath[lnklen] = '\0';
> + if (was_malloc)
> + free(path);
> + path = strdup(lnkpath);
This needs a NULL-check.
> + 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
>
fortune: allow to use symlinks
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 - 1.60
+++ fortune.c 14 Dec 2020 22:05:33 -
@@ -39,6 +39,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -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];
boolwas_malloc;
boolisdir;
@@ -420,8 +422,22 @@ add_file(int percent, char *file, char *
}
DPRINTF(1, (stderr, "adding file \"%s\"\n", path));
+
over:
+ lnklen = readlink(path, lnkpath, PATH_MAX);
+ if (lnklen == -1) {
+ if (errno != EINVAL)
+ goto openfailed;
+ } else {
+ lnkpath[lnklen] = '\0';
+ if (was_malloc)
+ free(path);
+ path = strdup(lnkpath);
+ 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
