weekly(8) isn't able to run locate.updatedb(8) anymore

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

weekly(8) isn't able to run locate.updatedb(8) anymore

Sebastien Marie-3
Hi,

Last change on locate.updatedb(8) ensures that the file passed via --fcodes
option is in a writeable directory, and error out if it isn't. but it
doesn't allow anymore '-' (output on stdout).

the use of '-' is documented has a valid option, and weekly(8) uses it
in order to run locate.updatedb(8) has 'nobody' and have the output in
tempfile under /var/db (place writable only by root).

with the change, as weekly(8) run locate.updatedb(8) with 2>/dev/null,
the error is silenciously discarded, letting the system without proper
/var/db/locate.database file.

# locate test
locate: database too small: /var/db/locate.database

The change should be discarded or at least permit '-' as valid option.

Thanks.
--
Sebastien Marie

Reply | Threaded
Open this post in threaded view
|

Re: weekly(8) isn't able to run locate.updatedb(8) anymore

Todd C. Miller-3
On Tue, 10 Sep 2019 09:42:16 +0200, Sebastien Marie wrote:

> the use of '-' is documented has a valid option, and weekly(8) uses it
> in order to run locate.updatedb(8) has 'nobody' and have the output in
> tempfile under /var/db (place writable only by root).
>
> with the change, as weekly(8) run locate.updatedb(8) with 2>/dev/null,
> the error is silenciously discarded, letting the system without proper
> /var/db/locate.database file.

Let's just skip that check when fcodes is "-".

 - todd

Index: usr.bin/locate/locate/updatedb.sh
===================================================================
RCS file: /cvs/src/usr.bin/locate/locate/updatedb.sh,v
retrieving revision 1.16
diff -u -p -u -r1.16 updatedb.sh
--- usr.bin/locate/locate/updatedb.sh 31 Aug 2019 16:03:28 -0000 1.16
+++ usr.bin/locate/locate/updatedb.sh 10 Sep 2019 15:06:02 -0000
@@ -70,10 +70,12 @@ while test $# != 0; do
  shift
 done
 
-FCODESDIR=$( dirname "${FCODES}" )
-if [ ! -w "${FCODESDIR}" ]; then
- echo "$0: no permission to create $FCODES"
- exit 1
+if [ "${FCODES}" != "-" ]; then
+ FCODESDIR=$( dirname "${FCODES}" )
+ if [ ! -w "${FCODESDIR}" ]; then
+ echo "$0: no permission to create $FCODES"
+ exit 1
+ fi
 fi
 
 case X"$SEARCHPATHS" in

Reply | Threaded
Open this post in threaded view
|

Re: weekly(8) isn't able to run locate.updatedb(8) anymore

Kurt Mosiejczuk-10
On Tue, Sep 10, 2019 at 09:10:57AM -0600, Todd C. Miller wrote:
> On Tue, 10 Sep 2019 09:42:16 +0200, Sebastien Marie wrote:

> > the use of '-' is documented has a valid option, and weekly(8) uses it
> > in order to run locate.updatedb(8) has 'nobody' and have the output in
> > tempfile under /var/db (place writable only by root).

> > with the change, as weekly(8) run locate.updatedb(8) with 2>/dev/null,
> > the error is silenciously discarded, letting the system without proper
> > /var/db/locate.database file.

> Let's just skip that check when fcodes is "-".

>  - todd

OK kmos for this change.

However, it looks like all that logic in weekly is unnecessary.
It is largely a duplicate of what is alkready in updatedb.sh.
updatedb.sh already traps more signals than weekly and uses a
tempfile just like weekly. The only thing it does not do is nice
itself.

Anyone have an argument to keep all the duplicate logic in weekly?
I know I may be missing something.

--Kurt

> Index: usr.bin/locate/locate/updatedb.sh
> ===================================================================
> RCS file: /cvs/src/usr.bin/locate/locate/updatedb.sh,v
> retrieving revision 1.16
> diff -u -p -u -r1.16 updatedb.sh
> --- usr.bin/locate/locate/updatedb.sh 31 Aug 2019 16:03:28 -0000 1.16
> +++ usr.bin/locate/locate/updatedb.sh 10 Sep 2019 15:06:02 -0000
> @@ -70,10 +70,12 @@ while test $# != 0; do
>   shift
>  done
>  
> -FCODESDIR=$( dirname "${FCODES}" )
> -if [ ! -w "${FCODESDIR}" ]; then
> - echo "$0: no permission to create $FCODES"
> - exit 1
> +if [ "${FCODES}" != "-" ]; then
> + FCODESDIR=$( dirname "${FCODES}" )
> + if [ ! -w "${FCODESDIR}" ]; then
> + echo "$0: no permission to create $FCODES"
> + exit 1
> + fi
>  fi
>  
>  case X"$SEARCHPATHS" in

Reply | Threaded
Open this post in threaded view
|

Re: weekly(8) isn't able to run locate.updatedb(8) anymore

Raf Czlonka-2
In reply to this post by Todd C. Miller-3
On Tue, Sep 10, 2019 at 04:10:57PM BST, Todd C. Miller wrote:

> On Tue, 10 Sep 2019 09:42:16 +0200, Sebastien Marie wrote:
>
> > the use of '-' is documented has a valid option, and weekly(8) uses it
> > in order to run locate.updatedb(8) has 'nobody' and have the output in
> > tempfile under /var/db (place writable only by root).
> >
> > with the change, as weekly(8) run locate.updatedb(8) with 2>/dev/null,
> > the error is silenciously discarded, letting the system without proper
> > /var/db/locate.database file.
>
> Let's just skip that check when fcodes is "-".
>
>  - todd
>
> Index: usr.bin/locate/locate/updatedb.sh
> ===================================================================
> RCS file: /cvs/src/usr.bin/locate/locate/updatedb.sh,v
> retrieving revision 1.16
> diff -u -p -u -r1.16 updatedb.sh
> --- usr.bin/locate/locate/updatedb.sh 31 Aug 2019 16:03:28 -0000 1.16
> +++ usr.bin/locate/locate/updatedb.sh 10 Sep 2019 15:06:02 -0000
> @@ -70,10 +70,12 @@ while test $# != 0; do
>   shift
>  done
>  
> -FCODESDIR=$( dirname "${FCODES}" )
> -if [ ! -w "${FCODESDIR}" ]; then
> - echo "$0: no permission to create $FCODES"
> - exit 1
> +if [ "${FCODES}" != "-" ]; then
> + FCODESDIR=$( dirname "${FCODES}" )
> + if [ ! -w "${FCODESDIR}" ]; then
> + echo "$0: no permission to create $FCODES"
> + exit 1
> + fi
>  fi
>  
>  case X"$SEARCHPATHS" in
>

How about ditching dirname(1)?

Index: usr.bin/locate/locate/updatedb.sh
===================================================================
RCS file: /cvs/src/usr.bin/locate/locate/updatedb.sh,v
retrieving revision 1.16
diff -u -p -u -r1.16 updatedb.sh
--- usr.bin/locate/locate/updatedb.sh 31 Aug 2019 16:03:28 -0000 1.16
+++ usr.bin/locate/locate/updatedb.sh 10 Sep 2019 15:06:02 -0000
@@ -70,10 +70,12 @@ while test $# != 0; do
  shift
 done
 
-FCODESDIR=$( dirname "${FCODES}" )
-if [ ! -w "${FCODESDIR}" ]; then
- echo "$0: no permission to create $FCODES"
- exit 1
+if [ "${FCODES}" != "-" ]; then
+ FCODESDIR="${FCODES%/*}"
+ if [ ! -w "${FCODESDIR}" ]; then
+ echo "$0: no permission to create $FCODES"
+ exit 1
+ fi
 fi
 
 case X"$SEARCHPATHS" in

Reply | Threaded
Open this post in threaded view
|

Re: weekly(8) isn't able to run locate.updatedb(8) anymore

Kurt Mosiejczuk-10
On Tue, Sep 10, 2019 at 04:56:42PM +0100, Raf Czlonka wrote:

> How about ditching dirname(1)?

File may not exist yet, in which case -w will be false, even if we
have permissions to create it.

--Kurt

> Index: usr.bin/locate/locate/updatedb.sh
> ===================================================================
> RCS file: /cvs/src/usr.bin/locate/locate/updatedb.sh,v
> retrieving revision 1.16
> diff -u -p -u -r1.16 updatedb.sh
> --- usr.bin/locate/locate/updatedb.sh 31 Aug 2019 16:03:28 -0000 1.16
> +++ usr.bin/locate/locate/updatedb.sh 10 Sep 2019 15:06:02 -0000
> @@ -70,10 +70,12 @@ while test $# != 0; do
>   shift
>  done
>  
> -FCODESDIR=$( dirname "${FCODES}" )
> -if [ ! -w "${FCODESDIR}" ]; then
> - echo "$0: no permission to create $FCODES"
> - exit 1
> +if [ "${FCODES}" != "-" ]; then
> + FCODESDIR="${FCODES%/*}"
> + if [ ! -w "${FCODESDIR}" ]; then
> + echo "$0: no permission to create $FCODES"
> + exit 1
> + fi
>  fi
>  
>  case X"$SEARCHPATHS" in
>

Reply | Threaded
Open this post in threaded view
|

Re: weekly(8) isn't able to run locate.updatedb(8) anymore

Raf Czlonka-2
On Tue, Sep 10, 2019 at 04:58:19PM BST, Kurt Mosiejczuk wrote:
> On Tue, Sep 10, 2019 at 04:56:42PM +0100, Raf Czlonka wrote:
>
> > How about ditching dirname(1)?
>
> File may not exist yet, in which case -w will be false, even if we
> have permissions to create it.

As per below, only dirname(1), not the logic! :^)

${FCODES%/*} instead of $( dirname "${FCODES}" )

Cheers,

Raf

> --Kurt
>
> > Index: usr.bin/locate/locate/updatedb.sh
> > ===================================================================
> > RCS file: /cvs/src/usr.bin/locate/locate/updatedb.sh,v
> > retrieving revision 1.16
> > diff -u -p -u -r1.16 updatedb.sh
> > --- usr.bin/locate/locate/updatedb.sh 31 Aug 2019 16:03:28 -0000 1.16
> > +++ usr.bin/locate/locate/updatedb.sh 10 Sep 2019 15:06:02 -0000
> > @@ -70,10 +70,12 @@ while test $# != 0; do
> >   shift
> >  done
> >  
> > -FCODESDIR=$( dirname "${FCODES}" )
> > -if [ ! -w "${FCODESDIR}" ]; then
> > - echo "$0: no permission to create $FCODES"
> > - exit 1
> > +if [ "${FCODES}" != "-" ]; then
> > + FCODESDIR="${FCODES%/*}"
> > + if [ ! -w "${FCODESDIR}" ]; then
> > + echo "$0: no permission to create $FCODES"
> > + exit 1
> > + fi
> >  fi
> >  
> >  case X"$SEARCHPATHS" in
> >

Reply | Threaded
Open this post in threaded view
|

Re: weekly(8) isn't able to run locate.updatedb(8) anymore

Kurt Mosiejczuk-10
On Tue, Sep 10, 2019 at 05:03:54PM +0100, Raf Czlonka wrote:
> On Tue, Sep 10, 2019 at 04:58:19PM BST, Kurt Mosiejczuk wrote:
> > On Tue, Sep 10, 2019 at 04:56:42PM +0100, Raf Czlonka wrote:

> > > How about ditching dirname(1)?

> > File may not exist yet, in which case -w will be false, even if we
> > have permissions to create it.

> As per below, only dirname(1), not the logic! :^)

> ${FCODES%/*} instead of $( dirname "${FCODES}" )

Is that portable outside of ksh? We had taken the non-ksh specific
route before to keep it portable. I see our sh(1) says it works, but
references I'm finding for plain bourne shell aren't showing me
${FOO%pattern}.

--Kurt

Reply | Threaded
Open this post in threaded view
|

Re: weekly(8) isn't able to run locate.updatedb(8) anymore

Raf Czlonka-2
On Tue, Sep 10, 2019 at 05:13:31PM BST, Kurt Mosiejczuk wrote:

> On Tue, Sep 10, 2019 at 05:03:54PM +0100, Raf Czlonka wrote:
> > On Tue, Sep 10, 2019 at 04:58:19PM BST, Kurt Mosiejczuk wrote:
> > > On Tue, Sep 10, 2019 at 04:56:42PM +0100, Raf Czlonka wrote:
>
> > > > How about ditching dirname(1)?
>
> > > File may not exist yet, in which case -w will be false, even if we
> > > have permissions to create it.
>
> > As per below, only dirname(1), not the logic! :^)
>
> > ${FCODES%/*} instead of $( dirname "${FCODES}" )
>
> Is that portable outside of ksh? We had taken the non-ksh specific
> route before to keep it portable. I see our sh(1) says it works, but
> references I'm finding for plain bourne shell aren't showing me
> ${FOO%pattern}.
>

Sure, it's POSIX[0] - ${parameter%word}

[0] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02

Regards,

Raf

Reply | Threaded
Open this post in threaded view
|

Re: weekly(8) isn't able to run locate.updatedb(8) anymore

Andreas Kusalananda Kähäri-3
In reply to this post by Kurt Mosiejczuk-10
On Tue, Sep 10, 2019 at 12:13:31PM -0400, Kurt Mosiejczuk wrote:

> On Tue, Sep 10, 2019 at 05:03:54PM +0100, Raf Czlonka wrote:
> > On Tue, Sep 10, 2019 at 04:58:19PM BST, Kurt Mosiejczuk wrote:
> > > On Tue, Sep 10, 2019 at 04:56:42PM +0100, Raf Czlonka wrote:
>
> > > > How about ditching dirname(1)?
>
> > > File may not exist yet, in which case -w will be false, even if we
> > > have permissions to create it.
>
> > As per below, only dirname(1), not the logic! :^)
>
> > ${FCODES%/*} instead of $( dirname "${FCODES}" )
>
> Is that portable outside of ksh? We had taken the non-ksh specific
> route before to keep it portable. I see our sh(1) says it works, but
> references I'm finding for plain bourne shell aren't showing me
> ${FOO%pattern}.

I don't know about Stephen Bourne's original sh, but ${variable%pattern}
is a POSIX expansion:

    https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/utilities/V3_chap02.html#tag_18_06_02

>
> --Kurt

Reply | Threaded
Open this post in threaded view
|

Re: weekly(8) isn't able to run locate.updatedb(8) anymore

Kurt Mosiejczuk-10
In reply to this post by Raf Czlonka-2
On Tue, Sep 10, 2019 at 05:22:33PM +0100, Raf Czlonka wrote:

> > > As per below, only dirname(1), not the logic! :^)

> > > ${FCODES%/*} instead of $( dirname "${FCODES}" )

> > Is that portable outside of ksh? We had taken the non-ksh specific
> > route before to keep it portable. I see our sh(1) says it works, but
> > references I'm finding for plain bourne shell aren't showing me
> > ${FOO%pattern}.

> Sure, it's POSIX[0] - ${parameter%word}

> [0] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02

In that case, I'd go with your version of it.

OK kmos

--Kurt

Reply | Threaded
Open this post in threaded view
|

Re: weekly(8) isn't able to run locate.updatedb(8) anymore

Todd C. Miller-3
In reply to this post by Raf Czlonka-2
On Tue, 10 Sep 2019 16:56:42 +0100, Raf Czlonka wrote:

> How about ditching dirname(1)?

dirname(1) handles files in the root directory as well as files
with no directory correctly, your diff does not.

E.g.

FCODES=/foo
${FCODES%/*} -> ""
dirname $FCODES -> "/"

FCODES=bar
${FCODES%/*} -> ""
dirname $FCODES -> "."

I think it best to keep dirname rather than try to reimplement it
in the shell.

 - todd

Reply | Threaded
Open this post in threaded view
|

Re: weekly(8) isn't able to run locate.updatedb(8) anymore

Raf Czlonka-2
On Tue, Sep 10, 2019 at 05:53:34PM BST, Todd C. Miller wrote:

> On Tue, 10 Sep 2019 16:56:42 +0100, Raf Czlonka wrote:
>
> > How about ditching dirname(1)?
>
> dirname(1) handles files in the root directory as well as files
> with no directory correctly, your diff does not.
>
> E.g.
>
> FCODES=/foo
> ${FCODES%/*} -> ""
> dirname $FCODES -> "/"
>
> FCODES=bar
> ${FCODES%/*} -> ""
> dirname $FCODES -> "."
>
> I think it best to keep dirname rather than try to reimplement it
> in the shell.
>
>  - todd

Fair enough.

FYI, what prompted me to use shell instead of dirname(1) is the
recent removal of basename(1)[0] but it seems like the latter doesn't
suffer from similar edge cases :^)

[0] http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/etc/rc.d/rc.subr.diff?r1=1.131&r2=1.132&sortby=date&f=h

Thanks for pointing that out!

Raf

Reply | Threaded
Open this post in threaded view
|

Re: weekly(8) isn't able to run locate.updatedb(8) anymore

Sebastien Marie-3
In reply to this post by Kurt Mosiejczuk-10
On Tue, Sep 10, 2019 at 11:51:09AM -0400, Kurt Mosiejczuk wrote:

> On Tue, Sep 10, 2019 at 09:10:57AM -0600, Todd C. Miller wrote:
>
> However, it looks like all that logic in weekly is unnecessary.
> It is largely a duplicate of what is alkready in updatedb.sh.
> updatedb.sh already traps more signals than weekly and uses a
> tempfile just like weekly. The only thing it does not do is nice
> itself.
>
> Anyone have an argument to keep all the duplicate logic in weekly?
> I know I may be missing something.

privsep.

weekly(8) is run as root, but it starts locate.updatedb(8) as 'nobody'.
so 'nobody' would have to have write perm to /var/db to create temporary
file, and you din't want that.
--
Sebastien Marie

Reply | Threaded
Open this post in threaded view
|

Re: weekly(8) isn't able to run locate.updatedb(8) anymore

Klemens Nanni-2
In reply to this post by Raf Czlonka-2
On Tue, Sep 10, 2019 at 05:58:53PM +0100, Raf Czlonka wrote:
> FYI, what prompted me to use shell instead of dirname(1) is the
> recent removal of basename(1)[0] but it seems like the latter doesn't
> suffer from similar edge cases :^)
Correct, because we can expect dirname(1) to be sure when weekly(8) is
running;  rc(8) runs much earlier when /usr may not be mounted yet.

Reply | Threaded
Open this post in threaded view
|

Re: weekly(8) isn't able to run locate.updatedb(8) anymore

Klemens Nanni-2
In reply to this post by Todd C. Miller-3
On Tue, Sep 10, 2019 at 09:10:57AM -0600, Todd C. Miller wrote:
> Let's just skip that check when fcodes is "-".
OK kn

Reply | Threaded
Open this post in threaded view
|

Re: weekly(8) isn't able to run locate.updatedb(8) anymore

Sebastien Marie-3
In reply to this post by Todd C. Miller-3
On Tue, Sep 10, 2019 at 09:10:57AM -0600, Todd C. Miller wrote:
>
> Let's just skip that check when fcodes is "-".
>

I'am fine with it too.

ok semarie@

>
> Index: usr.bin/locate/locate/updatedb.sh
> ===================================================================
> RCS file: /cvs/src/usr.bin/locate/locate/updatedb.sh,v
> retrieving revision 1.16
> diff -u -p -u -r1.16 updatedb.sh
> --- usr.bin/locate/locate/updatedb.sh 31 Aug 2019 16:03:28 -0000 1.16
> +++ usr.bin/locate/locate/updatedb.sh 10 Sep 2019 15:06:02 -0000
> @@ -70,10 +70,12 @@ while test $# != 0; do
>   shift
>  done
>  
> -FCODESDIR=$( dirname "${FCODES}" )
> -if [ ! -w "${FCODESDIR}" ]; then
> - echo "$0: no permission to create $FCODES"
> - exit 1
> +if [ "${FCODES}" != "-" ]; then
> + FCODESDIR=$( dirname "${FCODES}" )
> + if [ ! -w "${FCODESDIR}" ]; then
> + echo "$0: no permission to create $FCODES"
> + exit 1
> + fi
>  fi
>  
>  case X"$SEARCHPATHS" in

--
Sebastien Marie

Reply | Threaded
Open this post in threaded view
|

Re: weekly(8) isn't able to run locate.updatedb(8) anymore

Andreas Kusalananda Kähäri-3
In reply to this post by Raf Czlonka-2
On Tue, Sep 10, 2019 at 05:58:53PM +0100, Raf Czlonka wrote:

> On Tue, Sep 10, 2019 at 05:53:34PM BST, Todd C. Miller wrote:
> > On Tue, 10 Sep 2019 16:56:42 +0100, Raf Czlonka wrote:
> >
> > > How about ditching dirname(1)?
> >
> > dirname(1) handles files in the root directory as well as files
> > with no directory correctly, your diff does not.
> >
> > E.g.
> >
> > FCODES=/foo
> > ${FCODES%/*} -> ""
> > dirname $FCODES -> "/"
> >
> > FCODES=bar
> > ${FCODES%/*} -> ""
> > dirname $FCODES -> "."
> >
> > I think it best to keep dirname rather than try to reimplement it
> > in the shell.
> >
> >  - todd
>
> Fair enough.
>
> FYI, what prompted me to use shell instead of dirname(1) is the
> recent removal of basename(1)[0] but it seems like the latter doesn't
> suffer from similar edge cases :^)

The only potential issue with basename versus chopping things off until
the last slash with a parameter substitution is if you're dealing with
paths to directories and you're not sure whether there may be a slash
at the end or not. If there is, basename will give you "/a/b/" --> "b"
while the substitution would give you an empty string.


>
> [0] http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/etc/rc.d/rc.subr.diff?r1=1.131&r2=1.132&sortby=date&f=h
>
> Thanks for pointing that out!
>
> Raf

Reply | Threaded
Open this post in threaded view
|

Re: weekly(8) isn't able to run locate.updatedb(8) anymore

Antoine Jacoutot-7
In reply to this post by Raf Czlonka-2
On Tue, Sep 10, 2019 at 05:58:53PM +0100, Raf Czlonka wrote:

> On Tue, Sep 10, 2019 at 05:53:34PM BST, Todd C. Miller wrote:
> > On Tue, 10 Sep 2019 16:56:42 +0100, Raf Czlonka wrote:
> >
> > > How about ditching dirname(1)?
> >
> > dirname(1) handles files in the root directory as well as files
> > with no directory correctly, your diff does not.
> >
> > E.g.
> >
> > FCODES=/foo
> > ${FCODES%/*} -> ""
> > dirname $FCODES -> "/"
> >
> > FCODES=bar
> > ${FCODES%/*} -> ""
> > dirname $FCODES -> "."
> >
> > I think it best to keep dirname rather than try to reimplement it
> > in the shell.
> >
> >  - todd
>
> Fair enough.
>
> FYI, what prompted me to use shell instead of dirname(1) is the
> recent removal of basename(1)[0] but it seems like the latter doesn't
> suffer from similar edge cases :^)
>
> [0] http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/etc/rc.d/rc.subr.diff?r1=1.131&r2=1.132&sortby=date&f=h

Totally different use case / consequence.

--
Antoine