Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
On Mon, Jan 24, 2011 at 09:07:09PM +0100, Harald Jenny wrote: On Mon, Jan 24, 2011 at 08:59:41PM +0200, Teodor MICU wrote: 2011/1/24 Harald Jenny har...@a-little-linux-box.at: Ok although the PIDFILE line can be removed with the below code. I'm don't see where PIDFILE is removed. Just take a look at the next patch version. It looks OK to me too, with the grain of salt of not being an amavis user. Well I don't think this package will make it into Debian Squeeze as for me a prerequisite is a fixed libmilter version... Reading debian-release, seems that there is no consensus about removing amavisd-milter from squeeze. If you tested things carefully and everything was OK, I'd say upload. That should not make things worse that before, but better, starting real life check. And release team can still remove amavisd-milter from squeeze if needed. If a fixed libmilter version is a pre-requisite for this particular bug report, do not forget to set the corresponding blocker in the BTS. sorry when I kept you from doing other more release-critical work, this was not my intention :-(. No problem. Regards, -- Agustin -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
Hi, 2011/1/24 Harald Jenny har...@a-little-linux-box.at: On Mon, Jan 24, 2011 at 10:40:06PM +0200, Teodor MICU wrote: I can only spot some cosmetic issues, otherwise I see no problem. The change USER - SYSTEMUSER only makes the diff larger and not really necessary. I tend to disagree as USER (as well as USERNAME) is set by the shell and so a [ -n $USER ] || USER=amavis check always evaluates to the user running the init script :-/ - as this is not the desired action I decided to rename USER to SYSTEMUSER (and will also change this in the config file). Good catch. Indeed, $USER is defined on interactive sessions and I was only thinking about starting at boot. Ok, I see how MILTERSOCKET is used now. IMHO it's better to keep the number of command line options as short as possible. I can only agree in principle. Well I don't think this package will make it into Debian Squeeze as for me a prerequisite is a fixed libmilter version... sorry when I kept you from doing other more release-critical work, this was not my intention :-(. I tend to disagree here (as Agustin). If this was the case the package would have the 'squeeze-will-remove' tag at least until now. For what I know amavisd is used in Debian SMTP infrastructure which might be a strong reason to accept this small fix in Debian 6.0. If there are other issues (ie. with libmilter) that's another issue which might indeed cause its removal. Thanks -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
On Tue, Jan 25, 2011 at 04:57:42PM +0200, Teodor MICU wrote: Hi, Hello 2011/1/24 Harald Jenny har...@a-little-linux-box.at: On Mon, Jan 24, 2011 at 10:40:06PM +0200, Teodor MICU wrote: I can only spot some cosmetic issues, otherwise I see no problem. The change USER - SYSTEMUSER only makes the diff larger and not really necessary. I tend to disagree as USER (as well as USERNAME) is set by the shell and so a [ -n $USER ] || USER=amavis check always evaluates to the user running the init script :-/ - as this is not the desired action I decided to rename USER to SYSTEMUSER (and will also change this in the config file). Good catch. Indeed, $USER is defined on interactive sessions and I was only thinking about starting at boot. Yes therefor I think this change is also necessary do you agree? Ok, I see how MILTERSOCKET is used now. IMHO it's better to keep the number of command line options as short as possible. I can only agree in principle. :-) thanks Well I don't think this package will make it into Debian Squeeze as for me a prerequisite is a fixed libmilter version... sorry when I kept you from doing other more release-critical work, this was not my intention :-(. I tend to disagree here (as Agustin). If this was the case the package would have the 'squeeze-will-remove' tag at least until now. For what I know amavisd is used in Debian SMTP infrastructure which might be a strong reason to accept this small fix in Debian 6.0. If there are other issues (ie. with libmilter) that's another issue which might indeed cause its removal. As far as I know there are not many installations using amavisd-new with milter so I guess the impact should be fairly minimal - my main objection currently is that I have seen no amavisd-milter installation which runs stable with the current libmilter version in testing and I do not see any interest of the release team in fixing it. Thanks Thanks for your valuable input Harald -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
2011/1/25 Harald Jenny har...@a-little-linux-box.at: On Tue, Jan 25, 2011 at 04:57:42PM +0200, Teodor MICU wrote: Good catch. Indeed, $USER is defined on interactive sessions and I was only thinking about starting at boot. Yes therefor I think this change is also necessary do you agree? Yes. I tend to disagree here (as Agustin). If this was the case the package would have the 'squeeze-will-remove' tag at least until now. For what I know amavisd is used in Debian SMTP infrastructure which might be a strong reason to accept this small fix in Debian 6.0. If there are other issues (ie. with libmilter) that's another issue which might indeed cause its removal. As far as I know there are not many installations using amavisd-new with milter so I guess the impact should be fairly minimal - my main objection currently is that I have seen no amavisd-milter installation which runs stable with the current libmilter version in testing and I do not see any interest of the release team in fixing it. If you're absolutely sure that amavisd-milter is broken due to libmilter (in squeeze) than you should request its removal by the release team. Of course the alternative is to fix libmilter but this might have to wait for r1. Thanks -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
On Tue, Jan 25, 2011 at 02:16:51PM +0100, Agustin Martin wrote: On Mon, Jan 24, 2011 at 09:07:09PM +0100, Harald Jenny wrote: On Mon, Jan 24, 2011 at 08:59:41PM +0200, Teodor MICU wrote: 2011/1/24 Harald Jenny har...@a-little-linux-box.at: Ok although the PIDFILE line can be removed with the below code. I'm don't see where PIDFILE is removed. Just take a look at the next patch version. It looks OK to me too, with the grain of salt of not being an amavis user. :-) no problem Well I don't think this package will make it into Debian Squeeze as for me a prerequisite is a fixed libmilter version... Reading debian-release, seems that there is no consensus about removing amavisd-milter from squeeze. Well consensus may be nice to be achieved but not a necessity. If you tested things carefully and everything was OK, I'd say upload. I still have to do some tests yet. That should not make things worse that before, but better, starting real life check. Yes but before I will have to clean my old release branch which was rejected. And release team can still remove amavisd-milter from squeeze if needed. True yes If a fixed libmilter version is a pre-requisite for this particular bug report, do not forget to set the corresponding blocker in the BTS. http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=527862 this bug is already fixed in Debian unstable but there was no request from the maintainer to unblock it in time so the only method would be a t-p-u which I proposed about three weeks ago. sorry when I kept you from doing other more release-critical work, this was not my intention :-(. No problem. Thanks very much for your help! Regards, -- Agustin Kind regards Harald -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
On Tue, Jan 25, 2011 at 11:51:30PM +0200, Teodor MICU wrote: 2011/1/25 Harald Jenny har...@a-little-linux-box.at: On Tue, Jan 25, 2011 at 04:57:42PM +0200, Teodor MICU wrote: Good catch. Indeed, $USER is defined on interactive sessions and I was only thinking about starting at boot. Yes therefor I think this change is also necessary do you agree? Yes. Ok thanks I tend to disagree here (as Agustin). If this was the case the package would have the 'squeeze-will-remove' tag at least until now. For what I know amavisd is used in Debian SMTP infrastructure which might be a strong reason to accept this small fix in Debian 6.0. If there are other issues (ie. with libmilter) that's another issue which might indeed cause its removal. As far as I know there are not many installations using amavisd-new with milter so I guess the impact should be fairly minimal - my main objection currently is that I have seen no amavisd-milter installation which runs stable with the current libmilter version in testing and I do not see any interest of the release team in fixing it. If you're absolutely sure that amavisd-milter is broken due to libmilter (in squeeze) than you should request its removal by the release team. Yes I am I currently run it with my t-p-u to fix this (and also received the reply of a user which says this). Of course the alternative is to fix libmilter but this might have to wait for r1. Yes but this would have not been really necessary IMHO: http://lists.debian.org/debian-release/2011/01/msg00111.html Thanks Bye Harald -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
On Tue, Jan 18, 2011 at 11:57:37AM +0100, Julien Cristau wrote: On Thu, Jan 13, 2011 at 10:04:14 +0100, Harald Jenny wrote: Dear Gabor Kiss, thanks for the information, will test it myself and then release a new version. And thanks for your good bug report. Can this be fixed ASAP please? Cheers, Julien Dear Julien Cristau and release team, as http://lists.debian.org/debian-release/2011/01/msg00111.html and http://lists.debian.org/debian-release/2011/01/msg00498.html have yet been unanswered I guess that there won't be a fix for http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=527862 available in Debian Squeeze which makes amavisd-milter not ready for release. So could you please remove this package from Debian testing (which will also make this bug less time-critical to be solved.) Thanks for your help and kind regards Harald Jenny -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
Harald Jenny schrieb am Montag, den 24. Januar 2011: On Tue, Jan 18, 2011 at 11:57:37AM +0100, Julien Cristau wrote: On Thu, Jan 13, 2011 at 10:04:14 +0100, Harald Jenny wrote: Dear Gabor Kiss, thanks for the information, will test it myself and then release a new version. And thanks for your good bug report. Can this be fixed ASAP please? Dear Julien Cristau and release team, as http://lists.debian.org/debian-release/2011/01/msg00111.html and http://lists.debian.org/debian-release/2011/01/msg00498.html have yet been unanswered I guess that there won't be a fix for http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=527862 available in Debian Squeeze which makes amavisd-milter not ready for release. So could you please remove this package from Debian testing (which will also make this bug less time-critical to be solved.) Removing is bad bad idea as it leaves amavis user without a working milter. Alex -- Alexander Wirt, formo...@formorer.de CC99 2DDD D39E 75B0 B0AA B25C D35B BC99 BC7D 020A -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
Removing is bad bad idea as it leaves amavis user without a working milter. Alex Dear Alex After Squeeze release backports of both libmilter and amavisd-milter will be made available by me so people will be able to use them. Kind regards Harald Jenny -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
Hi Teodor Micu I had a look at the new patch and I have a few observations. Thanks 1) This construction is fine and simple as recommended in /etc/init.d/skeleton: # Read configuration variable file if it is present [ -r /etc/default/$NAME ] . /etc/default/$NAME Ok will look at it. 2) This construction doesn't work if PIDFILE is empty (not defined): +if [ $PIDFILE != /var/run/amavis/$NAME.pid ]; then Correct so I will check this too. For all strings comparison you need to use $VAR. Good point, missed this. 3) This seems to be valid but I've always used [ -n $VAR ] instead of: +if [ $MILTERSOCKET ]; then Will look at this too. 4) This doesn't work if the directory has a space: + if [ ! -e $(dirname $PIDFILE) ]; then +mkdir $(dirname $PIDFILE) You need to use $(dirname $PIDFILE). This is an issue if this variable can be defined to any custom value. Agreed, will be fixed. 5) This has a mistake: and extra ) character at the end + chown $USER:$(id $USER -g -n) $(dirname $PIDFILE)) Yes seems I missed this too... The correct execution would be: + chown $USER:$(id $USER -g -n) $(dirname $PIDFILE) Thanks for this hint. 6) I'm not sure this is working properly, I've always used [ -n -a.. ]. +if [ $MILTERSOCKET -a `echo... ]; then Ok Also, there is no need to check if MILTERSOCKET is empty in this case. I tend to disagree here as MILTERSOCKET can be defined in /etc/default/amavisd-milter (this is rather a guard against misconfiguration here). 7) You should probably add -q for all these executions to avoid unwanted strings during start/stop/restart. `echo $MILTERSOCKET | grep -v ^inet` If MILTERSOCKET is checked to contain text too? 8) The same as #5 for this construction: + if [ ! -e $(dirname $MILTERSOCKET) ]; then +mkdir $(dirname $MILTERSOCKET) + fi + chown $USER $(dirname $MILTERSOCKET)) Ok It is unclear to me why you need to change the owner of MILTERSOCKET. Because amavisd-milter running as user can't otherwise remove the socket when being stopped. Let me know if you need some help to fix this bug. I'm not using amavisd anymore but I could help in this case. Thanks I will post a new patch later and would really appreciate if you could take a look at it. Thanks Thanks Harald Jenny -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
Hello Agustin Martin Note that ''||' chains are different, will only fail if all components fail. However, chaining too much also affects readability. Ok thanks for this hint. It indeed looks better to me, thanks. While I did not test myself I think you may have problems with empty $MILTERSOCKET with dash as interpreter in if [ $MILTERSOCKET -a `echo $MILTERSOCKET | grep -v ^inet` ] as in e.g., 8 test #!/bin/sh bs=bs if [ $as -a $bs ]; then echo Hi fi 8--- end test $ dash test [: 5: -a: unexpected operator Quoting variables should help. Ok will check and fix this. Also I am not sure of full portability of -a there (although seems to not be a problem with dash). No I specifically checked the manpage for this. Maintainer scripts normally use chained '' in the if clause (note that this is different from standalone '' chains and is not a problem under 'set -e') I'd write above line as if [ $MILTERSOCKET ] [ `echo $MILTERSOCKET | grep -v ^inet` ]; then but as Teodor points out (just read it), second check seems to be enough. Not really as an empty $MILTERSOCKET may lead to undesired behaviour too. Also, I'd do a more extensive variable quoting, if [ $MILTERSOCKET ]; then if [ $AMAVISSOCKET ]; then ... besides those Teodor points out. Thanks will do this. -- Agustin Kind regards Harald Jenny -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
Hello On Fri, Jan 21, 2011 at 03:13:05PM +0200, Teodor MICU wrote: 2011/1/21 Agustin Martin agmar...@debian.org: if [ $MILTERSOCKET ] [ `echo $MILTERSOCKET | grep -v ^inet` ]; then but as Teodor points out (just read it), second check seems to be enough. Only that I realized latter the intention of this construction. My previous suggestion was to use this construction on its own without test: if echo $MILTERSOCKET | grep -q -v ^inet; then ... fi (the return code is set by grep) This works too but there is an extra test for the empty string: if [ $(echo $MILTERSOCKET | grep -v ^inet) ]; then ... fi (the return code is set by 'test') Will check this deeper. Thanks Thank you Harald -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
Hi Augustin On Sun, Jan 23, 2011 at 01:38:47AM +0100, Agustin Martin wrote: 2011/1/21 Teodor MICU mteo...@gmail.com: 2011/1/21 Agustin Martin agmar...@debian.org: if [ $MILTERSOCKET ] [ `echo $MILTERSOCKET | grep -v ^inet` ]; then but as Teodor points out (just read it), second check seems to be enough. Only that I realized latter the intention of this construction. My previous suggestion was to use this construction on its own without test: if echo $MILTERSOCKET | grep -q -v ^inet; then ... fi (the return code is set by grep) This works too but there is an extra test for the empty string: if [ $(echo $MILTERSOCKET | grep -v ^inet) ]; then ... fi (the return code is set by 'test') If I understand correctly, first expression saves redundant checks, so is preferrable. I will have a to check this - this is meant as a guard against accidently setting $MILTERSOCKET to . Harald, I've rewritten patch suggestion with more quoting and Teodor's check (Teodor, please check that I understood correctly) . Thanks Instead of repeating the check a variable is used for next similar check, but that was the way I played, feel free to act at your convenience. Will take a look at this today (as work permits). I do not use amavisd myself, so patch is completely unchecked, just verified that 'sh -n' on the script gives no errors. Check on this is appreciated. Ok thank you very much. Patch is attached, Thanks again. Cheers, -- Agustin Kind regards Harald diff -Nru --exclude changelog amavisd-milter-1.5.0/debian/amavisd-milter.init amavisd-milter-1.5.0/debian/amavisd-milter.init --- amavisd-milter-1.5.0/debian/amavisd-milter.init 2010-05-12 23:01:42.0 +0200 +++ amavisd-milter-1.5.0/debian/amavisd-milter.init 2011-01-22 20:40:46.0 +0100 @@ -30,19 +30,43 @@ OPTIONS= # Exit if the package is not installed -[ -x $DAEMON ] || exit 0 +[ -x $DAEMON ] || exit 0 # Read configuration variable file if it is present -[ -r /etc/default/$NAME ] . /etc/default/$NAME - -[ $PIDFILE != /var/run/amavis/$NAME.pid ] OPTIONS=$OPTIONS -p $PIDFILE -[ $MILTERSOCKET ] OPTIONS=$OPTIONS -s $MILTERSOCKET -[ $AMAVISSOCKET ] OPTIONS=$OPTIONS -S $AMAVISSOCKET -[ $WORKINGDIR ] OPTIONS=$OPTIONS -w $WORKINGDIR -[ $EXTRAPARAMS ] OPTIONS=$OPTIONS $EXTRAPARAMS - -[ $PIDFILE ] ([ -d $(dirname $PIDFILE) ] || mkdir $(dirname $PIDFILE) chown $USER:$(id $USER -g -n) $(dirname $PIDFILE)) -[ $MILTERSOCKET ] ([ -d $(dirname $MILTERSOCKET) ] || mkdir $(dirname $MILTERSOCKET) chown $USER $(dirname $MILTERSOCKET)) +if [ -r /etc/default/$NAME ]; then + . /etc/default/$NAME +fi + +if [ $PIDFILE != /var/run/amavis/$NAME.pid ]; then + OPTIONS=$OPTIONS -p $PIDFILE +fi +if [ $MILTERSOCKET ]; then + OPTIONS=$OPTIONS -s $MILTERSOCKET +fi +if [ $AMAVISSOCKET ]; then + OPTIONS=$OPTIONS -S $AMAVISSOCKET +fi +if [ $WORKINGDIR ]; then + OPTIONS=$OPTIONS -w $WORKINGDIR +fi +if [ $EXTRAPARAMS ]; then + OPTIONS=$OPTIONS $EXTRAPARAMS +fi + +if [ $PIDFILE ]; then + if [ ! -e $(dirname $PIDFILE) ]; then +mkdir $(dirname $PIDFILE) + fi + chown $USER:$(id $USER -g -n) $(dirname $PIDFILE) +fi +unset NOINET_MILTERSOCKET +if echo $MILTERSOCKET | grep -q -v ^inet; then + if [ ! -e $(dirname $MILTERSOCKET) ]; then +mkdir $(dirname $MILTERSOCKET) + fi + NOINET_MILTERSOCKET=1 + chown $USER $(dirname $MILTERSOCKET) +fi START=--start --quiet --chuid $USER --pidfile $PIDFILE --startas $DAEMON --name $NAME -- $OPTIONS STOP=--stop --quiet --retry 10 --pidfile $PIDFILE --startas $DAEMON --name $NAME @@ -53,25 +77,51 @@ case $1 in start) log_daemon_msg Starting $DESC: $NAME - start-stop-daemon $START + start-stop-daemon $START + case $? in - 0) log_end_msg 0 -[ $MILTERSOCKET ] [ $MILTERSOCKETOWNER ] chown $MILTERSOCKETOWNER $MILTERSOCKET -[ $MILTERSOCKET ] [ $MILTERSOCKETMODE ] chmod $MILTERSOCKETMODE $MILTERSOCKET ;; - 1) log_progress_msg already started -log_end_msg 0 ;; - *) log_end_msg $? ;; + 0) +log_end_msg 0 +if [ $NOINET_MILTERSOCKET ]; then + if [ $MILTERSOCKETOWNER ]; then +chown $MILTERSOCKETOWNER $MILTERSOCKET + fi + if [ $MILTERSOCKETMODE ]; then +chmod $MILTERSOCKETMODE $MILTERSOCKET + fi +fi +;; + + 1) +log_progress_msg already started +log_end_msg 0 +;; + + *) +log_end_msg $? +;; + esac ;; stop) log_daemon_msg Stopping $DESC: $NAME start-stop-daemon $STOP + case $? in - 0) log_end_msg 0 ;; -
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
On Mon, Jan 24, 2011 at 02:17:48PM +0100, Harald Jenny wrote: Hi Augustin On Sun, Jan 23, 2011 at 01:38:47AM +0100, Agustin Martin wrote: 2011/1/21 Teodor MICU mteo...@gmail.com: 2011/1/21 Agustin Martin agmar...@debian.org: if [ $MILTERSOCKET ] [ `echo $MILTERSOCKET | grep -v ^inet` ]; then but as Teodor points out (just read it), second check seems to be enough. Only that I realized latter the intention of this construction. My previous suggestion was to use this construction on its own without test: if echo $MILTERSOCKET | grep -q -v ^inet; then ... fi (the return code is set by grep) This works too but there is an extra test for the empty string: if [ $(echo $MILTERSOCKET | grep -v ^inet) ]; then ... fi (the return code is set by 'test') If I understand correctly, first expression saves redundant checks, so is preferrable. I will have a to check this - this is meant as a guard against accidently setting $MILTERSOCKET to . Damm, seems I wrongly understood Theodor's mail, and my proposed change will not work because of echo newline. Anyway, in the case above, 'echo -n' (allowed by policy 10.4) http://www.de.debian.org/doc/debian-policy/ch-files.html#s-scripts should help with that, if echo -n $MILTERSOCKET | grep -q -v ^inet; then That will still not protect against things like setting $MILTERSOCKET to , but dealing with is a must. -- Agustin -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
On Mon, Jan 24, 2011 at 02:08:08PM +0100, Harald Jenny wrote: Hello Agustin Martin Also I am not sure of full portability of -a there (although seems to not be a problem with dash). No I specifically checked the manpage for this. You are right. Furthermore, just noticed that -a and -o are explicitly allowed by policy 10.4) http://www.de.debian.org/doc/debian-policy/ch-files.html#s-scripts -- Agustin -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
On Mon, Jan 24, 2011 at 03:54:49PM +0100, Agustin Martin wrote: On Mon, Jan 24, 2011 at 02:08:08PM +0100, Harald Jenny wrote: Hello Agustin Martin Also I am not sure of full portability of -a there (although seems to not be a problem with dash). No I specifically checked the manpage for this. You are right. Furthermore, just noticed that -a and -o are explicitly allowed by policy 10.4) http://www.de.debian.org/doc/debian-policy/ch-files.html#s-scripts Ok thanks for the pointer, but I will presumably change this anyway to keep the coding style consistent. -- Agustin Harald -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
On Mon, Jan 24, 2011 at 03:50:04PM +0100, Agustin Martin wrote: On Mon, Jan 24, 2011 at 02:17:48PM +0100, Harald Jenny wrote: Hi Augustin On Sun, Jan 23, 2011 at 01:38:47AM +0100, Agustin Martin wrote: 2011/1/21 Teodor MICU mteo...@gmail.com: 2011/1/21 Agustin Martin agmar...@debian.org: if [ $MILTERSOCKET ] [ `echo $MILTERSOCKET | grep -v ^inet` ]; then but as Teodor points out (just read it), second check seems to be enough. Only that I realized latter the intention of this construction. My previous suggestion was to use this construction on its own without test: if echo $MILTERSOCKET | grep -q -v ^inet; then ... fi (the return code is set by grep) This works too but there is an extra test for the empty string: if [ $(echo $MILTERSOCKET | grep -v ^inet) ]; then ... fi (the return code is set by 'test') If I understand correctly, first expression saves redundant checks, so is preferrable. I will have a to check this - this is meant as a guard against accidently setting $MILTERSOCKET to . Damm, seems I wrongly understood Theodor's mail, and my proposed change will not work because of echo newline. Anyway, in the case above, 'echo -n' (allowed by policy 10.4) http://www.de.debian.org/doc/debian-policy/ch-files.html#s-scripts should help with that, Thanks for the pointer if echo -n $MILTERSOCKET | grep -q -v ^inet; then That will still not protect against things like setting $MILTERSOCKET to , but dealing with is a must. Yes it is, will upload a new version of my patch ASAP. -- Agustin Harald -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
Hi, 2011/1/24 Harald Jenny har...@a-little-linux-box.at: 7) You should probably add -q for all these executions to avoid unwanted strings during start/stop/restart. `echo $MILTERSOCKET | grep -v ^inet` If MILTERSOCKET is checked to contain text too? Yes, it does cover the case where MILTERSOCKET contains something but not if it is empty. For this I would recommend 'printf' since 'echo -n' is not portable and not working with /bin/dash. It is unclear to me why you need to change the owner of MILTERSOCKET. Because amavisd-milter running as user can't otherwise remove the socket when being stopped. Well, I was thinking that there should be a better way to create the socket with the correct owner and group. This is just an workaround for being created by root. I think spamass-millter does a pretty good job here since I use it in the default configuration (via a socket rather than inet). Thanks -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
Hi, 2011/1/24 Agustin Martin agmar...@debian.org: On Mon, Jan 24, 2011 at 02:17:48PM +0100, Harald Jenny wrote: I will have a to check this - this is meant as a guard against accidently setting $MILTERSOCKET to . Good catch on this. I didn't though of this being empty. Damm, seems I wrongly understood Theodor's mail, and my proposed change will not work because of echo newline. Anyway, in the case above, 'echo -n' (allowed by policy 10.4) http://www.de.debian.org/doc/debian-policy/ch-files.html#s-scripts should help with that, if echo -n $MILTERSOCKET | grep -q -v ^inet; then That will still not protect against things like setting $MILTERSOCKET to , but dealing with is a must. I don't recommend to use echo with any parameters since it is not portable. The only reliable call to echo is just echo. These calls are not working reliable: echo -n, echo -e, echo -en. For any of these construction just use printf but very important to guard the strings with . So the recommended check to catch all combinations would be: | if printf $MILTERSOCKET | grep -q -v ^inet; then | ... | fi Also, the echo -n is one of the bashism constructions that don't work with dash behind /bin/sh (aka = squeeze). Thanks -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
On Mon, Jan 24, 2011 at 05:09:10PM +0200, Teodor MICU wrote: Hi, Hello 2011/1/24 Harald Jenny har...@a-little-linux-box.at: 7) You should probably add -q for all these executions to avoid unwanted strings during start/stop/restart. `echo $MILTERSOCKET | grep -v ^inet` If MILTERSOCKET is checked to contain text too? Yes, it does cover the case where MILTERSOCKET contains something but not if it is empty. For this I would recommend 'printf' since 'echo -n' is not portable and not working with /bin/dash. I will soon post another version of the patch could you please take a look at it? As /bin/sh is used for this script dash compatibility is an important point. It is unclear to me why you need to change the owner of MILTERSOCKET. Because amavisd-milter running as user can't otherwise remove the socket when being stopped. Well, I was thinking that there should be a better way to create the socket with the correct owner and group. This is just an workaround for being created by root. I think spamass-millter does a pretty good job here since I use it in the default configuration (via a socket rather than inet). Well this workaround is not necessary for sendmail but for postfix. Thanks Thank you Harald -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
On Mon, Jan 24, 2011 at 17:09:10 +0200, Teodor MICU wrote: Hi, 2011/1/24 Harald Jenny har...@a-little-linux-box.at: 7) You should probably add -q for all these executions to avoid unwanted strings during start/stop/restart. `echo $MILTERSOCKET | grep -v ^inet` If MILTERSOCKET is checked to contain text too? Yes, it does cover the case where MILTERSOCKET contains something but not if it is empty. For this I would recommend 'printf' since 'echo -n' is not portable and not working with /bin/dash. Eh, what? Scripts may assume that `/bin/sh' implements the SUSv3 Shell Command Language[1] plus the following additional features not mandated by SUSv3:[2] * `echo -n', if implemented as a shell built-in, must not generate a newline. Works with dash afaict. Cheers, Julien signature.asc Description: Digital signature
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
On Mon, Jan 24, 2011 at 04:20:32PM +0100, Julien Cristau wrote: On Mon, Jan 24, 2011 at 17:09:10 +0200, Teodor MICU wrote: Hi, 2011/1/24 Harald Jenny har...@a-little-linux-box.at: 7) You should probably add -q for all these executions to avoid unwanted strings during start/stop/restart. `echo $MILTERSOCKET | grep -v ^inet` If MILTERSOCKET is checked to contain text too? Yes, it does cover the case where MILTERSOCKET contains something but not if it is empty. For this I would recommend 'printf' since 'echo -n' is not portable and not working with /bin/dash. Eh, what? Scripts may assume that `/bin/sh' implements the SUSv3 Shell Command Language[1] plus the following additional features not mandated by SUSv3:[2] * `echo -n', if implemented as a shell built-in, must not generate a newline. Works with dash afaict. Yes works Cheers, Julien Thanks Harald -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
Hi, 2011/1/24 Julien Cristau jcris...@debian.org: Yes, it does cover the case where MILTERSOCKET contains something but not if it is empty. For this I would recommend 'printf' since 'echo -n' is not portable and not working with /bin/dash. Eh, what? Scripts may assume that `/bin/sh' implements the SUSv3 Shell Command Language[1] plus the following additional features not mandated by SUSv3:[2] * `echo -n', if implemented as a shell built-in, must not generate a newline. Works with dash afaict. Last time I checked I was testing several systems (not only on Debian) and I had some issues with echo -n. Ever since I've replace it with 'printf' but you might be right that echo -n is working fine with dash too. I know for user that I'll stick to this convention since I don't have Debian on all hosts. Thanks -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
On Mon, Jan 24, 2011 at 05:18:15PM +0200, Teodor MICU wrote: Hi, Hello 2011/1/24 Agustin Martin agmar...@debian.org: On Mon, Jan 24, 2011 at 02:17:48PM +0100, Harald Jenny wrote: I will have a to check this - this is meant as a guard against accidently setting $MILTERSOCKET to . Good catch on this. I didn't though of this being empty. ;-) did it myself already so... Damm, seems I wrongly understood Theodor's mail, and my proposed change will not work because of echo newline. Anyway, in the case above, 'echo -n' (allowed by policy 10.4) http://www.de.debian.org/doc/debian-policy/ch-files.html#s-scripts should help with that, if echo -n $MILTERSOCKET | grep -q -v ^inet; then That will still not protect against things like setting $MILTERSOCKET to , but dealing with is a must. I don't recommend to use echo with any parameters since it is not portable. The only reliable call to echo is just echo. These calls are not working reliable: echo -n, echo -e, echo -en. For any of these construction just use printf but very important to guard the strings with . So the recommended check to catch all combinations would be: | if printf $MILTERSOCKET | grep -q -v ^inet; then | ... | fi Also, the echo -n is one of the bashism constructions that don't work with dash behind /bin/sh (aka = squeeze). cat /etc/debian_version 6.0 \h:\w$ echo -n X X\h:\w$ Works with sid at least. Thanks Wish you a nice day Harald -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
On Mon, Jan 24, 2011 at 05:28:13PM +0200, Teodor MICU wrote: Hi, Hey 2011/1/24 Julien Cristau jcris...@debian.org: Yes, it does cover the case where MILTERSOCKET contains something but not if it is empty. For this I would recommend 'printf' since 'echo -n' is not portable and not working with /bin/dash. Eh, what? Scripts may assume that `/bin/sh' implements the SUSv3 Shell Command Language[1] plus the following additional features not mandated by SUSv3:[2] * `echo -n', if implemented as a shell built-in, must not generate a newline. Works with dash afaict. Last time I checked I was testing several systems (not only on Debian) and I had some issues with echo -n. Ever since I've replace it with 'printf' but you might be right that echo -n is working fine with dash too. I know for user that I'll stick to this convention since I don't have Debian on all hosts. Well as far as I know Debian currently only supports /bin/bash and /bin/dash as providers of /bin/sh so I guess it's currently safe to use echo -n in init scripts. Thanks Thanks for you input Harald -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
2011/1/24 Harald Jenny har...@a-little-linux-box.at: Well as far as I know Debian currently only supports /bin/bash and /bin/dash as providers of /bin/sh so I guess it's currently safe to use echo -n in init scripts. It's fine. After some digging I guess I had in mind echo -e and echo -en are not working with dash but it seems that echo -n (without -e) has no issues. Thanks -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
Hi all, first thanks to everbody for the valuable input, it helped me a lot to improve this init script. Please take a look at the third version of my patch and comment on it. Thanks and a nice day Harald --- /etc/init.d/amavisd-milter 2010-05-12 23:01:42.0 +0200 +++ /etc/init.d/amavisd-milter 2011-01-24 15:57:08.0 +0100 @@ -21,57 +21,103 @@ # Description: milter daemon for sendmail/postfix and amavisd-new ### END INIT INFO +# Define LSB functions. +. /lib/lsb/init-functions + PATH=/sbin:/bin:/usr/sbin:/usr/bin DAEMON=/usr/sbin/amavisd-milter NAME=amavisd-milter DESC=amavisd-milter Daemon USER=amavis -PIDFILE=/var/run/amavis/$NAME.pid +PIDFILE=/var/run/amavis/$NAME.pid OPTIONS= # Exit if the package is not installed -[ -x $DAEMON ] || exit 0 +[ -x $DAEMON ] || exit 0 # Read configuration variable file if it is present -[ -r /etc/default/$NAME ] . /etc/default/$NAME +[ -r /etc/default/$NAME ] . /etc/default/$NAME -[ $PIDFILE != /var/run/amavis/$NAME.pid ] OPTIONS=$OPTIONS -p $PIDFILE -[ $MILTERSOCKET ] OPTIONS=$OPTIONS -s $MILTERSOCKET -[ $AMAVISSOCKET ] OPTIONS=$OPTIONS -S $AMAVISSOCKET -[ $WORKINGDIR ] OPTIONS=$OPTIONS -w $WORKINGDIR -[ $EXTRAPARAMS ] OPTIONS=$OPTIONS $EXTRAPARAMS - -[ $PIDFILE ] ([ -d $(dirname $PIDFILE) ] || mkdir $(dirname $PIDFILE) chown $USER:$(id $USER -g -n) $(dirname $PIDFILE)) -[ $MILTERSOCKET ] ([ -d $(dirname $MILTERSOCKET) ] || mkdir $(dirname $MILTERSOCKET) chown $USER $(dirname $MILTERSOCKET)) +if [ -n $PIDFILE ]; then + if [ $PIDFILE != /var/run/amavis/$NAME.pid ]; then +OPTIONS=$OPTIONS -p $PIDFILE + fi + if [ ! -e $(dirname $PIDFILE) ]; then +mkdir $(dirname $PIDFILE) + fi + chown $USER:$(id $USER -g -n) $(dirname $PIDFILE) +else + log_failure_msg Error: PIDFILE variable must be defined for correct functionality + exit 1 +fi + +if [ -n $MILTERSOCKET ]; then + OPTIONS=$OPTIONS -s $MILTERSOCKET + if [ $(echo $MILTERSOCKET | grep ^inet) ]; then +MILTERSOCKETTYPE=tcp + else +MILTERSOCKETTYPE=pipe +if [ ! -e $(dirname $MILTERSOCKET) ]; then + mkdir $(dirname $MILTERSOCKET) +fi +chown $USER $(dirname $MILTERSOCKET) + fi +fi + +[ -n $AMAVISSOCKET ] OPTIONS=$OPTIONS -S $AMAVISSOCKET +[ -n $WORKINGDIR ] OPTIONS=$OPTIONS -w $WORKINGDIR +[ -n $EXTRAPARAMS ] OPTIONS=$OPTIONS $EXTRAPARAMS START=--start --quiet --chuid $USER --pidfile $PIDFILE --startas $DAEMON --name $NAME -- $OPTIONS STOP=--stop --quiet --retry 10 --pidfile $PIDFILE --startas $DAEMON --name $NAME -# Define LSB functions. -. /lib/lsb/init-functions - case $1 in start) log_daemon_msg Starting $DESC: $NAME - start-stop-daemon $START + start-stop-daemon $START + case $? in - 0) log_end_msg 0 - [ $MILTERSOCKET ] [ $MILTERSOCKETOWNER ] chown $MILTERSOCKETOWNER $MILTERSOCKET - [ $MILTERSOCKET ] [ $MILTERSOCKETMODE ] chmod $MILTERSOCKETMODE $MILTERSOCKET ;; - 1) log_progress_msg already started - log_end_msg 0 ;; - *) log_end_msg $? ;; + 0) + log_end_msg 0 + if [ $MILTERSOCKETTYPE = pipe ]; then + if [ $MILTERSOCKETOWNER ]; then + chown $MILTERSOCKETOWNER $MILTERSOCKET + fi + if [ $MILTERSOCKETMODE ]; then + chmod $MILTERSOCKETMODE $MILTERSOCKET + fi + fi + ;; + + 1) + log_progress_msg already started + log_end_msg 0 + ;; + + *) + log_end_msg $? + ;; + esac ;; stop) log_daemon_msg Stopping $DESC: $NAME start-stop-daemon $STOP + case $? in - 0) log_end_msg 0 ;; - 1) log_progress_msg already stopped - log_end_msg 0 ;; - *) log_end_msg $? ;; + 0) + log_end_msg 0 + ;; + + 1) + log_progress_msg already stopped + log_end_msg 0 + ;; + + *) log_end_msg $? + ;; + esac ;;
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
On Mon, Jan 24, 2011 at 05:50:38PM +0200, Teodor MICU wrote: 2011/1/24 Harald Jenny har...@a-little-linux-box.at: Well as far as I know Debian currently only supports /bin/bash and /bin/dash as providers of /bin/sh so I guess it's currently safe to use echo -n in init scripts. It's fine. After some digging I guess I had in mind echo -e and echo -en are not working with dash but it seems that echo -n (without -e) has no issues. At least my little tests seem to indicate this ;-). Thanks Bye Harald -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
Hi again, 2011/1/24 Harald Jenny har...@a-little-linux-box.at: first thanks to everbody for the valuable input, it helped me a lot to improve this init script. Please take a look at the third version of my patch and comment on it. Overall it seems fine, just a few observations: 1) usually you should enclose with the full path here: +PIDFILE=/var/run/amavis/$NAME.pid +[ -r /etc/default/$NAME ] . /etc/default/$NAME Also, $NAME is not usually quoted with in any init script. 2) You should make sure that you don't get it this situation: +if [ -n $PIDFILE ]; then ... +else + log_failure_msg Error: PIDFILE variable must be defined for correct functionality + exit 1 +fi This is usually done by setting the default values (for example for PIDFILE) after you have sourced the config from /etc/default/$NAME. This is done on a test like this: test -z $VAR || VAR=your default value (I don't recommend test -n $VAR .. not that good with set -e) You should consider all variables essential to amavisd-milter startup and runtime. 3) Replace these to work with set -e: +[ -n $AMAVISSOCKET ] .. with +[ -z $AMAVISSOCKET ] || .. Thanks -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
On Mon, Jan 24, 2011 at 06:37:37PM +0200, Teodor MICU wrote: Hi again, Hello again :-) 2011/1/24 Harald Jenny har...@a-little-linux-box.at: first thanks to everbody for the valuable input, it helped me a lot to improve this init script. Please take a look at the third version of my patch and comment on it. Overall it seems fine, just a few observations: 1) usually you should enclose with the full path here: +PIDFILE=/var/run/amavis/$NAME.pid +[ -r /etc/default/$NAME ] . /etc/default/$NAME You mean like this? PIDFILE=/var/run/amavis/$NAME.pid [ -r /etc/default/$NAME ] . /etc/default/$NAME Also, $NAME is not usually quoted with in any init script. Well that's true but you may set it to a value with blanks in it too so maybe the defaults should also be changed here? If you think it's not necessary I may also drop in but I wanted to be on the safe side here. 2) You should make sure that you don't get it this situation: +if [ -n $PIDFILE ]; then ... +else + log_failure_msg Error: PIDFILE variable must be defined for correct functionality + exit 1 +fi This is usually done by setting the default values (for example for PIDFILE) after you have sourced the config from /etc/default/$NAME. This is done on a test like this: test -z $VAR || VAR=your default value (I don't recommend test -n $VAR .. not that good with set -e) You mean not alerting when PIDFILE is set to in the config file but rather changing it to my default value, correct? You should consider all variables essential to amavisd-milter startup and runtime. 3) Replace these to work with set -e: +[ -n $AMAVISSOCKET ] .. with +[ -z $AMAVISSOCKET ] || .. Hmmm ok Thanks Thank you Harald -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
2011/1/24 Harald Jenny har...@a-little-linux-box.at: On Mon, Jan 24, 2011 at 06:37:37PM +0200, Teodor MICU wrote: 1) usually you should enclose with the full path here: +PIDFILE=/var/run/amavis/$NAME.pid +[ -r /etc/default/$NAME ] . /etc/default/$NAME You mean like this? PIDFILE=/var/run/amavis/$NAME.pid [ -r /etc/default/$NAME ] . /etc/default/$NAME Yes. Also, $NAME is not usually quoted with in any init script. Well that's true but you may set it to a value with blanks in it too so maybe the defaults should also be changed here? If you think it's not necessary I may also drop in but I wanted to be on the safe side here. I don't think this is a supported configuration. I mean $NAME should probably not be changed in config from /etc/default but as you say it is good to be on the safe side. 2) You should make sure that you don't get it this situation: +if [ -n $PIDFILE ]; then ... +else + log_failure_msg Error: PIDFILE variable must be defined for correct functionality + exit 1 +fi This is usually done by setting the default values (for example for PIDFILE) after you have sourced the config from /etc/default/$NAME. This is done on a test like this: test -z $VAR || VAR=your default value (I don't recommend test -n $VAR .. not that good with set -e) You mean not alerting when PIDFILE is set to in the config file but rather changing it to my default value, correct? Yes. It should be a valid config if /etc/default/$NAME that doesn't contain anything. Actually it should be the default to have only comments with the default values that the SysAdmin should be able to change with custom values. I don't know what you have now in the default config. One more important issue I think we missed so far is to have MILTERSOCKET empty in the config file. I think you covered only to skip chmod+chown but in fact it should abort execution. So this construction: +if [ -n $MILTERSOCKET ]; then should be change to: | if [ -z $MILTERSOCKET ]; then | echo .. abort | exit 1 | else [..] Thanks -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
Just ignore this. I probably need some coffee. Having MILTERSOCKET variable empty is also with the case with the proposed configuration where you set your default unix:path/to/socket value. Thanks 2011/1/24 Teodor MICU mteo...@gmail.com: One more important issue I think we missed so far is to have MILTERSOCKET empty in the config file. I think you covered only to skip chmod+chown but in fact it should abort execution. So this construction: +if [ -n $MILTERSOCKET ]; then should be change to: | if [ -z $MILTERSOCKET ]; then | echo .. abort | exit 1 | else [..] -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
On Mon, Jan 24, 2011 at 07:22:17PM +0200, Teodor MICU wrote: 2011/1/24 Harald Jenny har...@a-little-linux-box.at: On Mon, Jan 24, 2011 at 06:37:37PM +0200, Teodor MICU wrote: 1) usually you should enclose with the full path here: +PIDFILE=/var/run/amavis/$NAME.pid +[ -r /etc/default/$NAME ] . /etc/default/$NAME You mean like this? PIDFILE=/var/run/amavis/$NAME.pid [ -r /etc/default/$NAME ] . /etc/default/$NAME Yes. Ok although the PIDFILE line can be removed with the below code. Also, $NAME is not usually quoted with in any init script. Well that's true but you may set it to a value with blanks in it too so maybe the defaults should also be changed here? If you think it's not necessary I may also drop in but I wanted to be on the safe side here. I don't think this is a supported configuration. I mean $NAME should probably not be changed in config from /etc/default but as you say it is good to be on the safe side. Yes *ggg* 2) You should make sure that you don't get it this situation: +if [ -n $PIDFILE ]; then ... +else + log_failure_msg Error: PIDFILE variable must be defined for correct functionality + exit 1 +fi This is usually done by setting the default values (for example for PIDFILE) after you have sourced the config from /etc/default/$NAME. This is done on a test like this: test -z $VAR || VAR=your default value (I don't recommend test -n $VAR .. not that good with set -e) You mean not alerting when PIDFILE is set to in the config file but rather changing it to my default value, correct? Yes. It should be a valid config if /etc/default/$NAME that doesn't contain anything. Actually it should be the default to have only comments with the default values that the SysAdmin should be able to change with custom values. I don't know what you have now in the default config. Well it's actually this way but PIDFILE needs to be set explicitly for the --pidfile option in the start-stop-daemon stanza - the binary itself already has the default path hardcoded. One more important issue I think we missed so far is to have MILTERSOCKET empty in the config file. I think you covered only to skip chmod+chown but in fact it should abort execution. So this construction: +if [ -n $MILTERSOCKET ]; then should be change to: | if [ -z $MILTERSOCKET ]; then | echo .. abort | exit 1 | else [..] Not really as there is a default value set in the binary which points to /var/lib/amavis/amavisd-milter.sock - I guess I should rather do: if [ -z $MILTERSOCKET ]; then MILTERSOCKETTYPE=pipe else OPTIONS=$OPTIONS -s $MILTERSOCKET if [ $(echo $MILTERSOCKET | grep ^inet) ]; then MILTERSOCKETTYPE=tcp else MILTERSOCKETTYPE=pipe if [ ! -e $(dirname $MILTERSOCKET) ]; then mkdir $(dirname $MILTERSOCKET) fi chown $USER $(dirname $MILTERSOCKET) fi fi as this would allow to also set MILTERSOCKETOWNER and MILTERSOCKETMODE with the default pipe location - what do you think? Thanks Thanks Harald -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
On Mon, Jan 24, 2011 at 07:29:09PM +0200, Teodor MICU wrote: Just ignore this. I probably need some coffee. Having MILTERSOCKET variable empty is also with the case with the proposed configuration where you set your default unix:path/to/socket value. Well I will make another upload soon. Thanks Bye Harald 2011/1/24 Teodor MICU mteo...@gmail.com: One more important issue I think we missed so far is to have MILTERSOCKET empty in the config file. I think you covered only to skip chmod+chown but in fact it should abort execution. So this construction: +if [ -n $MILTERSOCKET ]; then should be change to: | if [ -z $MILTERSOCKET ]; then | echo .. abort | exit 1 | else [..] -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
2011/1/24 Harald Jenny har...@a-little-linux-box.at: Ok although the PIDFILE line can be removed with the below code. I'm don't see where PIDFILE is removed. Yes. It should be a valid config if /etc/default/$NAME that doesn't contain anything. Actually it should be the default to have only comments with the default values that the SysAdmin should be able to change with custom values. I don't know what you have now in the default config. Well it's actually this way but PIDFILE needs to be set explicitly for the --pidfile option in the start-stop-daemon stanza - the binary itself already has the default path hardcoded. One more important issue I think we missed so far is to have MILTERSOCKET empty in the config file. I think you covered only to skip chmod+chown but in fact it should abort execution. So this construction: I was wrong here. MILTERSOCKET can be empty in the config from /etc/default/$NAME and this should be the default. Not really as there is a default value set in the binary which points to /var/lib/amavis/amavisd-milter.sock - I guess I should rather do: if [ -z $MILTERSOCKET ]; then MILTERSOCKETTYPE=pipe else [..] fi as this would allow to also set MILTERSOCKETOWNER and MILTERSOCKETMODE with the default pipe location - what do you think? I think the above test is always false if you correctly initialize MILTERSOCKET after the /etc/default/$NAME has been sourced. [ -z $MILTERSOCKET ] || MILTERSOCKET=/var/lib/amavis/amavisd-milter.sock Later on you don't check if it's empty but only the type: unix vs inet. Thanks -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
On Mon, Jan 24, 2011 at 08:59:41PM +0200, Teodor MICU wrote: 2011/1/24 Harald Jenny har...@a-little-linux-box.at: Ok although the PIDFILE line can be removed with the below code. I'm don't see where PIDFILE is removed. Just take a look at the next patch version. Yes. It should be a valid config if /etc/default/$NAME that doesn't contain anything. Actually it should be the default to have only comments with the default values that the SysAdmin should be able to change with custom values. I don't know what you have now in the default config. Well it's actually this way but PIDFILE needs to be set explicitly for the --pidfile option in the start-stop-daemon stanza - the binary itself already has the default path hardcoded. One more important issue I think we missed so far is to have MILTERSOCKET empty in the config file. I think you covered only to skip chmod+chown but in fact it should abort execution. So this construction: I was wrong here. MILTERSOCKET can be empty in the config from /etc/default/$NAME and this should be the default. Yes but if it's empty the default value is a unix socket. Not really as there is a default value set in the binary which points to /var/lib/amavis/amavisd-milter.sock - I guess I should rather do: if [ -z $MILTERSOCKET ]; then MILTERSOCKETTYPE=pipe else [..] fi as this would allow to also set MILTERSOCKETOWNER and MILTERSOCKETMODE with the default pipe location - what do you think? I think the above test is always false if you correctly initialize MILTERSOCKET after the /etc/default/$NAME has been sourced. [ -z $MILTERSOCKET ] || MILTERSOCKET=/var/lib/amavis/amavisd-milter.sock True but I only want to append to OPTIONS when the default values are overriden because this keeps the command line shorter :-). Later on you don't check if it's empty but only the type: unix vs inet. Well seems to me this is just a change in the code flow... Thanks Thanks for your feedback --- /etc/init.d/amavisd-milter 2010-05-12 23:01:42.0 +0200 +++ /etc/init.d/amavisd-milter 2011-01-24 20:47:02.0 +0100 @@ -25,26 +25,48 @@ DAEMON=/usr/sbin/amavisd-milter NAME=amavisd-milter DESC=amavisd-milter Daemon -USER=amavis -PIDFILE=/var/run/amavis/$NAME.pid OPTIONS= # Exit if the package is not installed -[ -x $DAEMON ] || exit 0 +[ -x $DAEMON ] || exit 0 # Read configuration variable file if it is present -[ -r /etc/default/$NAME ] . /etc/default/$NAME +[ -r /etc/default/$NAME ] . /etc/default/$NAME -[ $PIDFILE != /var/run/amavis/$NAME.pid ] OPTIONS=$OPTIONS -p $PIDFILE -[ $MILTERSOCKET ] OPTIONS=$OPTIONS -s $MILTERSOCKET -[ $AMAVISSOCKET ] OPTIONS=$OPTIONS -S $AMAVISSOCKET -[ $WORKINGDIR ] OPTIONS=$OPTIONS -w $WORKINGDIR -[ $EXTRAPARAMS ] OPTIONS=$OPTIONS $EXTRAPARAMS +[ -n $SYSTEMUSER ] || SYSTEMUSER=amavis -[ $PIDFILE ] ([ -d $(dirname $PIDFILE) ] || mkdir $(dirname $PIDFILE) chown $USER:$(id $USER -g -n) $(dirname $PIDFILE)) -[ $MILTERSOCKET ] ([ -d $(dirname $MILTERSOCKET) ] || mkdir $(dirname $MILTERSOCKET) chown $USER $(dirname $MILTERSOCKET)) +if [ -z $PIDFILE ]; then + PIDFILE=/var/run/amavis/$NAME.pid +else + OPTIONS=$OPTIONS -p $PIDFILE +fi +PIDDIR=$(dirname $PIDFILE) +if [ ! -e $PIDDIR ]; then + mkdir $PIDDIR + chown $SYSTEMUSER:$(id $SYSTEMUSER -g -n) $PIDDIR +fi + +if [ -z $MILTERSOCKET ]; then + MILTERSOCKETTYPE=pipe +else + OPTIONS=$OPTIONS -s $MILTERSOCKET + if [ $(echo $MILTERSOCKET | grep ^inet) ]; then +MILTERSOCKETTYPE=tcp + else +MILTERSOCKETTYPE=pipe +MILTERDIR=$(dirname $MILTERSOCKET) +if [ ! -e $MILTERDIR ]; then + mkdir $MILTERDIR + chown $SYSTEMUSER $MILTERDIR +fi + fi +fi + +[ -z $AMAVISSOCKET ] || OPTIONS=$OPTIONS -S $AMAVISSOCKET +[ -z $WORKINGDIR ] || OPTIONS=$OPTIONS -w $WORKINGDIR +[ -z $EXTRAPARAMS ] || OPTIONS=$OPTIONS $EXTRAPARAMS -START=--start --quiet --chuid $USER --pidfile $PIDFILE --startas $DAEMON --name $NAME -- $OPTIONS +START=--start --quiet --chuid $SYSTEMUSER --pidfile $PIDFILE --startas $DAEMON --name $NAME -- $OPTIONS STOP=--stop --quiet --retry 10 --pidfile $PIDFILE --startas $DAEMON --name $NAME # Define LSB functions. @@ -53,25 +75,50 @@ case $1 in start) log_daemon_msg Starting $DESC: $NAME - start-stop-daemon $START + start-stop-daemon $START + case $? in - 0) log_end_msg 0 - [ $MILTERSOCKET ] [ $MILTERSOCKETOWNER ] chown $MILTERSOCKETOWNER $MILTERSOCKET - [ $MILTERSOCKET ] [ $MILTERSOCKETMODE ] chmod $MILTERSOCKETMODE $MILTERSOCKET ;; - 1) log_progress_msg already started - log_end_msg 0 ;; - *) log_end_msg $? ;; + 0) + log_end_msg 0 + if [ $MILTERSOCKETTYPE = pipe ]; then + if [ $MILTERSOCKETOWNER ]; then + chown $MILTERSOCKETOWNER $MILTERSOCKET + fi + if [ $MILTERSOCKETMODE ]; then + chmod $MILTERSOCKETMODE $MILTERSOCKET + fi + fi + ;; + + 1) + log_progress_msg already started +
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
2011/1/24 Harald Jenny har...@a-little-linux-box.at: On Mon, Jan 24, 2011 at 08:59:41PM +0200, Teodor MICU wrote: 2011/1/24 Harald Jenny har...@a-little-linux-box.at: Ok although the PIDFILE line can be removed with the below code. I'm don't see where PIDFILE is removed. Just take a look at the next patch version. I can only spot some cosmetic issues, otherwise I see no problem. The change USER - SYSTEMUSER only makes the diff larger and not really necessary. It was a good idea to get the 'dirname' out on their own, it is cleaner like this. I was wrong here. MILTERSOCKET can be empty in the config from /etc/default/$NAME and this should be the default. Yes but if it's empty the default value is a unix socket. Ok. I think the above test is always false if you correctly initialize MILTERSOCKET after the /etc/default/$NAME has been sourced. [ -z $MILTERSOCKET ] || MILTERSOCKET=/var/lib/amavis/amavisd-milter.sock True but I only want to append to OPTIONS when the default values are overriden because this keeps the command line shorter :-). Ok, I see how MILTERSOCKET is used now. Later on you don't check if it's empty but only the type: unix vs inet. Well seems to me this is just a change in the code flow... Yes. The release team will check too, maybe we missed something. :) Thanks -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
On Mon, Jan 24, 2011 at 10:40:06PM +0200, Teodor MICU wrote: 2011/1/24 Harald Jenny har...@a-little-linux-box.at: On Mon, Jan 24, 2011 at 08:59:41PM +0200, Teodor MICU wrote: 2011/1/24 Harald Jenny har...@a-little-linux-box.at: Ok although the PIDFILE line can be removed with the below code. I'm don't see where PIDFILE is removed. Just take a look at the next patch version. I can only spot some cosmetic issues, otherwise I see no problem. The change USER - SYSTEMUSER only makes the diff larger and not really necessary. I tend to disagree as USER (as well as USERNAME) is set by the shell and so a [ -n $USER ] || USER=amavis check always evaluates to the user running the init script :-/ - as this is not the desired action I decided to rename USER to SYSTEMUSER (and will also change this in the config file). It was a good idea to get the 'dirname' out on their own, it is cleaner like this. Thanks :-) I was wrong here. MILTERSOCKET can be empty in the config from /etc/default/$NAME and this should be the default. Yes but if it's empty the default value is a unix socket. Ok. The default value for the binary is defined when doing ./configure I think the above test is always false if you correctly initialize MILTERSOCKET after the /etc/default/$NAME has been sourced. [ -z $MILTERSOCKET ] || MILTERSOCKET=/var/lib/amavis/amavisd-milter.sock True but I only want to append to OPTIONS when the default values are overriden because this keeps the command line shorter :-). Ok, I see how MILTERSOCKET is used now. IMHO it's better to keep the number of command line options as short as possible. Later on you don't check if it's empty but only the type: unix vs inet. Well seems to me this is just a change in the code flow... Yes. Ok The release team will check too, maybe we missed something. :) Well I don't think this package will make it into Debian Squeeze as for me a prerequisite is a fixed libmilter version... sorry when I kept you from doing other more release-critical work, this was not my intention :-(. Thanks Bye and good night Harald -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
2011/1/21 Teodor MICU mteo...@gmail.com: 2011/1/21 Agustin Martin agmar...@debian.org: if [ $MILTERSOCKET ] [ `echo $MILTERSOCKET | grep -v ^inet` ]; then but as Teodor points out (just read it), second check seems to be enough. Only that I realized latter the intention of this construction. My previous suggestion was to use this construction on its own without test: if echo $MILTERSOCKET | grep -q -v ^inet; then ... fi (the return code is set by grep) This works too but there is an extra test for the empty string: if [ $(echo $MILTERSOCKET | grep -v ^inet) ]; then ... fi (the return code is set by 'test') If I understand correctly, first expression saves redundant checks, so is preferrable. Harald, I've rewritten patch suggestion with more quoting and Teodor's check (Teodor, please check that I understood correctly) . Instead of repeating the check a variable is used for next similar check, but that was the way I played, feel free to act at your convenience. I do not use amavisd myself, so patch is completely unchecked, just verified that 'sh -n' on the script gives no errors. Check on this is appreciated. Patch is attached, Cheers, -- Agustin diff -Nru --exclude changelog amavisd-milter-1.5.0/debian/amavisd-milter.init amavisd-milter-1.5.0/debian/amavisd-milter.init --- amavisd-milter-1.5.0/debian/amavisd-milter.init 2010-05-12 23:01:42.0 +0200 +++ amavisd-milter-1.5.0/debian/amavisd-milter.init 2011-01-22 20:40:46.0 +0100 @@ -30,19 +30,43 @@ OPTIONS= # Exit if the package is not installed -[ -x $DAEMON ] || exit 0 +[ -x $DAEMON ] || exit 0 # Read configuration variable file if it is present -[ -r /etc/default/$NAME ] . /etc/default/$NAME - -[ $PIDFILE != /var/run/amavis/$NAME.pid ] OPTIONS=$OPTIONS -p $PIDFILE -[ $MILTERSOCKET ] OPTIONS=$OPTIONS -s $MILTERSOCKET -[ $AMAVISSOCKET ] OPTIONS=$OPTIONS -S $AMAVISSOCKET -[ $WORKINGDIR ] OPTIONS=$OPTIONS -w $WORKINGDIR -[ $EXTRAPARAMS ] OPTIONS=$OPTIONS $EXTRAPARAMS - -[ $PIDFILE ] ([ -d $(dirname $PIDFILE) ] || mkdir $(dirname $PIDFILE) chown $USER:$(id $USER -g -n) $(dirname $PIDFILE)) -[ $MILTERSOCKET ] ([ -d $(dirname $MILTERSOCKET) ] || mkdir $(dirname $MILTERSOCKET) chown $USER $(dirname $MILTERSOCKET)) +if [ -r /etc/default/$NAME ]; then + . /etc/default/$NAME +fi + +if [ $PIDFILE != /var/run/amavis/$NAME.pid ]; then + OPTIONS=$OPTIONS -p $PIDFILE +fi +if [ $MILTERSOCKET ]; then + OPTIONS=$OPTIONS -s $MILTERSOCKET +fi +if [ $AMAVISSOCKET ]; then + OPTIONS=$OPTIONS -S $AMAVISSOCKET +fi +if [ $WORKINGDIR ]; then + OPTIONS=$OPTIONS -w $WORKINGDIR +fi +if [ $EXTRAPARAMS ]; then + OPTIONS=$OPTIONS $EXTRAPARAMS +fi + +if [ $PIDFILE ]; then + if [ ! -e $(dirname $PIDFILE) ]; then +mkdir $(dirname $PIDFILE) + fi + chown $USER:$(id $USER -g -n) $(dirname $PIDFILE) +fi +unset NOINET_MILTERSOCKET +if echo $MILTERSOCKET | grep -q -v ^inet; then + if [ ! -e $(dirname $MILTERSOCKET) ]; then +mkdir $(dirname $MILTERSOCKET) + fi + NOINET_MILTERSOCKET=1 + chown $USER $(dirname $MILTERSOCKET) +fi START=--start --quiet --chuid $USER --pidfile $PIDFILE --startas $DAEMON --name $NAME -- $OPTIONS STOP=--stop --quiet --retry 10 --pidfile $PIDFILE --startas $DAEMON --name $NAME @@ -53,25 +77,51 @@ case $1 in start) log_daemon_msg Starting $DESC: $NAME - start-stop-daemon $START + start-stop-daemon $START + case $? in - 0) log_end_msg 0 - [ $MILTERSOCKET ] [ $MILTERSOCKETOWNER ] chown $MILTERSOCKETOWNER $MILTERSOCKET - [ $MILTERSOCKET ] [ $MILTERSOCKETMODE ] chmod $MILTERSOCKETMODE $MILTERSOCKET ;; - 1) log_progress_msg already started - log_end_msg 0 ;; - *) log_end_msg $? ;; + 0) + log_end_msg 0 + if [ $NOINET_MILTERSOCKET ]; then + if [ $MILTERSOCKETOWNER ]; then + chown $MILTERSOCKETOWNER $MILTERSOCKET + fi + if [ $MILTERSOCKETMODE ]; then + chmod $MILTERSOCKETMODE $MILTERSOCKET + fi + fi + ;; + + 1) + log_progress_msg already started + log_end_msg 0 + ;; + + *) + log_end_msg $? + ;; + esac ;; stop) log_daemon_msg Stopping $DESC: $NAME start-stop-daemon $STOP + case $? in - 0) log_end_msg 0 ;; - 1) log_progress_msg already stopped - log_end_msg 0 ;; - *) log_end_msg $? ;; + 0) + log_end_msg 0 + ;; + + 1) + log_progress_msg already stopped + log_end_msg 0 + ;; + + *) + log_end_msg $? + ;; + esac ;;
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
[Harald please CC: my address otherwise I could not see your responses] 2011/1/20 Harald Jenny har...@a-little-linux-box.at: I checked with other init scripts an in order to have a consistent coding style in the init script I replaced the with if-clauses - could you take a look at it and tell me if this looks better to you? I had a look at the new patch and I have a few observations. 1) This construction is fine and simple as recommended in /etc/init.d/skeleton: # Read configuration variable file if it is present [ -r /etc/default/$NAME ] . /etc/default/$NAME 2) This construction doesn't work if PIDFILE is empty (not defined): +if [ $PIDFILE != /var/run/amavis/$NAME.pid ]; then For all strings comparison you need to use $VAR. 3) This seems to be valid but I've always used [ -n $VAR ] instead of: +if [ $MILTERSOCKET ]; then 4) This doesn't work if the directory has a space: + if [ ! -e $(dirname $PIDFILE) ]; then +mkdir $(dirname $PIDFILE) You need to use $(dirname $PIDFILE). This is an issue if this variable can be defined to any custom value. 5) This has a mistake: and extra ) character at the end + chown $USER:$(id $USER -g -n) $(dirname $PIDFILE)) The correct execution would be: + chown $USER:$(id $USER -g -n) $(dirname $PIDFILE) 6) I'm not sure this is working properly, I've always used [ -n -a.. ]. +if [ $MILTERSOCKET -a `echo... ]; then Also, there is no need to check if MILTERSOCKET is empty in this case. 7) You should probably add -q for all these executions to avoid unwanted strings during start/stop/restart. `echo $MILTERSOCKET | grep -v ^inet` 8) The same as #5 for this construction: + if [ ! -e $(dirname $MILTERSOCKET) ]; then +mkdir $(dirname $MILTERSOCKET) + fi + chown $USER $(dirname $MILTERSOCKET)) It is unclear to me why you need to change the owner of MILTERSOCKET. Let me know if you need some help to fix this bug. I'm not using amavisd anymore but I could help in this case. Thanks -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
On Thu, Jan 20, 2011 at 10:19:28PM +0100, Harald Jenny wrote: Hello Agustin Martin Typical problems with those chained '' appear when 'set -e' is used, but amavisd-milter init script does not use it. Ah ok thanks will keep this info in mind. Note that ''||' chains are different, will only fail if all components fail. However, chaining too much also affects readability. When there are more that two elements in the chain, I usually find these if-clauses way more readable and so easier to debug. I checked with other init scripts an in order to have a consistent coding style in the init script I replaced the with if-clauses - could you take a look at it and tell me if this looks better to you? It indeed looks better to me, thanks. While I did not test myself I think you may have problems with empty $MILTERSOCKET with dash as interpreter in if [ $MILTERSOCKET -a `echo $MILTERSOCKET | grep -v ^inet` ] as in e.g., 8 test #!/bin/sh bs=bs if [ $as -a $bs ]; then echo Hi fi 8--- end test $ dash test [: 5: -a: unexpected operator Quoting variables should help. Also I am not sure of full portability of -a there (although seems to not be a problem with dash). Maintainer scripts normally use chained '' in the if clause (note that this is different from standalone '' chains and is not a problem under 'set -e') I'd write above line as if [ $MILTERSOCKET ] [ `echo $MILTERSOCKET | grep -v ^inet` ]; then but as Teodor points out (just read it), second check seems to be enough. Also, I'd do a more extensive variable quoting, if [ $MILTERSOCKET ]; then if [ $AMAVISSOCKET ]; then ... besides those Teodor points out. -- Agustin -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
2011/1/21 Agustin Martin agmar...@debian.org: if [ $MILTERSOCKET ] [ `echo $MILTERSOCKET | grep -v ^inet` ]; then but as Teodor points out (just read it), second check seems to be enough. Only that I realized latter the intention of this construction. My previous suggestion was to use this construction on its own without test: if echo $MILTERSOCKET | grep -q -v ^inet; then ... fi (the return code is set by grep) This works too but there is an extra test for the empty string: if [ $(echo $MILTERSOCKET | grep -v ^inet) ]; then ... fi (the return code is set by 'test') Thanks -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
Hello Agustin Martin Typical problems with those chained '' appear when 'set -e' is used, but amavisd-milter init script does not use it. Ah ok thanks will keep this info in mind. When there are more that two elements in the chain, I usually find these if-clauses way more readable and so easier to debug. I checked with other init scripts an in order to have a consistent coding style in the init script I replaced the with if-clauses - could you take a look at it and tell me if this looks better to you? Hope this helps, Yes thanks it's very informative! -- Agustin Kind regards Harald Jenny --- /etc/init.d/amavisd-milter 2010-05-12 23:01:42.0 +0200 +++ /etc/init.d/amavisd-milter_FIXED 2011-01-20 21:22:45.0 +0100 @@ -33,16 +33,38 @@ [ -x $DAEMON ] || exit 0 # Read configuration variable file if it is present -[ -r /etc/default/$NAME ] . /etc/default/$NAME - -[ $PIDFILE != /var/run/amavis/$NAME.pid ] OPTIONS=$OPTIONS -p $PIDFILE -[ $MILTERSOCKET ] OPTIONS=$OPTIONS -s $MILTERSOCKET -[ $AMAVISSOCKET ] OPTIONS=$OPTIONS -S $AMAVISSOCKET -[ $WORKINGDIR ] OPTIONS=$OPTIONS -w $WORKINGDIR -[ $EXTRAPARAMS ] OPTIONS=$OPTIONS $EXTRAPARAMS - -[ $PIDFILE ] ([ -d $(dirname $PIDFILE) ] || mkdir $(dirname $PIDFILE) chown $USER:$(id $USER -g -n) $(dirname $PIDFILE)) -[ $MILTERSOCKET ] ([ -d $(dirname $MILTERSOCKET) ] || mkdir $(dirname $MILTERSOCKET) chown $USER $(dirname $MILTERSOCKET)) +if [ -r /etc/default/$NAME ]; then + . /etc/default/$NAME +fi + +if [ $PIDFILE != /var/run/amavis/$NAME.pid ]; then + OPTIONS=$OPTIONS -p $PIDFILE +fi +if [ $MILTERSOCKET ]; then + OPTIONS=$OPTIONS -s $MILTERSOCKET +fi +if [ $AMAVISSOCKET ]; then + OPTIONS=$OPTIONS -S $AMAVISSOCKET +fi +if [ $WORKINGDIR ]; then + OPTIONS=$OPTIONS -w $WORKINGDIR +fi +if [ $EXTRAPARAMS ]; then + OPTIONS=$OPTIONS $EXTRAPARAMS +fi + +if [ $PIDFILE ]; then + if [ ! -e $(dirname $PIDFILE) ]; then +mkdir $(dirname $PIDFILE) + fi + chown $USER:$(id $USER -g -n) $(dirname $PIDFILE)) +fi +if [ $MILTERSOCKET -a `echo $MILTERSOCKET | grep -v ^inet` ]; then + if [ ! -e $(dirname $MILTERSOCKET) ]; then +mkdir $(dirname $MILTERSOCKET) + fi + chown $USER $(dirname $MILTERSOCKET)) +fi START=--start --quiet --chuid $USER --pidfile $PIDFILE --startas $DAEMON --name $NAME -- $OPTIONS STOP=--stop --quiet --retry 10 --pidfile $PIDFILE --startas $DAEMON --name $NAME @@ -53,25 +75,51 @@ case $1 in start) log_daemon_msg Starting $DESC: $NAME - start-stop-daemon $START + start-stop-daemon $START + case $? in - 0) log_end_msg 0 - [ $MILTERSOCKET ] [ $MILTERSOCKETOWNER ] chown $MILTERSOCKETOWNER $MILTERSOCKET - [ $MILTERSOCKET ] [ $MILTERSOCKETMODE ] chmod $MILTERSOCKETMODE $MILTERSOCKET ;; - 1) log_progress_msg already started - log_end_msg 0 ;; - *) log_end_msg $? ;; + 0) + log_end_msg 0 + if [ $MILTERSOCKET -a `echo $MILTERSOCKET | grep -v ^inet` ]; then + if [ $MILTERSOCKETOWNER ]; then + chown $MILTERSOCKETOWNER $MILTERSOCKET + fi + if [ $MILTERSOCKETMODE ]; then + chmod $MILTERSOCKETMODE $MILTERSOCKET + fi + fi + ;; + + 1) + log_progress_msg already started + log_end_msg 0 + ;; + + *) + log_end_msg $? + ;; + esac ;; stop) log_daemon_msg Stopping $DESC: $NAME start-stop-daemon $STOP + case $? in - 0) log_end_msg 0 ;; - 1) log_progress_msg already stopped - log_end_msg 0 ;; - *) log_end_msg $? ;; + 0) + log_end_msg 0 + ;; + + 1) + log_progress_msg already stopped + log_end_msg 0 + ;; + + *) + log_end_msg $? + ;; + esac ;;
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
On Mon, Jan 17, 2011 at 12:28:06AM +0100, Harald Jenny wrote: Dear Teodor Micu thanks for looking at my patch and thanks for your mail, could you please give me an example how the sequences may fail as I've done some tests with it and it seems it catches all normal configurations, even when a user if specified for a network milter socket, Typical problems with those chained '' appear when 'set -e' is used, but amavisd-milter init script does not use it. and the alternative implementation here seems to consist of stacked if-clauses. When there are more that two elements in the chain, I usually find these if-clauses way more readable and so easier to debug. Hope this helps, -- Agustin -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
On Thu, Jan 13, 2011 at 10:04:14 +0100, Harald Jenny wrote: Dear Gabor Kiss, thanks for the information, will test it myself and then release a new version. And thanks for your good bug report. Can this be fixed ASAP please? Cheers, Julien signature.asc Description: Digital signature
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
Dear Teodor Micu
thanks for looking at my patch and thanks for your mail, could you please give
me an example how the sequences may fail as I've done some tests with it and
it seems it catches all normal configurations, even when a user if specified
for a network milter socket, and the alternative implementation here seems to
consist of stacked if-clauses.
Regarding the splitting of MILTERSOCKET into MILTERTYPE and MILTERSOCKET I'm
currently little bit undecided as the manpage of amavisd-milter is written like
this:
-s socket
Communication socket between sendmail and amavisd-milter (default
/var/lib/amavis/amavisd-milter.sock). The protocol spoken over this socket is
MILTER
(Mail FILTER). It must agree with the INPUT_MAIL_FILTER entry in
sendmail.mc
The socket should be in proto:address format:
· {unix|local}:/path/to/file - A named pipe.
· inet:port@{hostname|ip-address} - An IPV4 socket.
· inet6:port@{hostname|ip-address} - An IPV6 socket.
Splitting the MILTERSOCKET variable into two params may irritate users looking
into the documentation more than it helps avoiding potential problems, maybe we
should bring this question to debian-devel to get a broader audience for this
discussion?
Kind regards
Harald Jenny
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
Hi Harald, I looked at your patch and I think those multiple checks in chain are prone to mistakes in some conditions. I've reported the same problem with clamav-milter [1] some time ago and I believe that is a cleaner and better implementation for checking SOCKET, SOCKET_TYPE and SOCKET_PATH. I'm writing this to you to check the current script from /etc/init.d/clamav-milter and include something similar for amavisd-milter. Thanks [1] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=55 -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
Dear Gabor Kiss, thanks for the information, will test it myself and then release a new version. And thanks for your good bug report. Kind regards Harald Jenny -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
Package: amavisd-milter Version: 1.5.0-2 Severity: grave Tags: security Justification: user security hole After sudo bash I issued /etc/init.d/amavisd-milter restart. What a surprise! My home directory got owned by user amavis. Running init script under bash -vx reveals the problem: [ $MILTERSOCKET ] ([ -d $(dirname $MILTERSOCKET) ] || mkdir $(dirname $MILTERSOCKET) chown $USER $(dirname $MILTERSOCKET)) + '[' inet6:60001 ']' dirname $MILTERSOCKET ++ dirname inet6:60001 + '[' -d . ']' dirname $MILTERSOCKET ++ dirname inet6:60001 + chown amavis . Yes, of course: the root directory is also owned by amavis(!!!) due to the first boot process since installing amavisd-milter package. :-( And some other random directories too that were cwd when starting daemon by hand. Gabor -- System Information: Debian Release: 5.0.7 APT prefers stable APT policy: (700, 'stable'), (500, 'proposed-updates') Architecture: i386 (i686) Kernel: Linux 2.6.26-2-686 (SMP w/4 CPU cores) Locale: LANG=C, LC_CTYPE=C (charmap=ANSI_X3.4-1968) Shell: /bin/sh linked to /bin/bash Versions of packages amavisd-milter depends on: ii amavisd-new1:2.6.4-1~bpo50+1 Interface between MTA and virus sc ii libc6 2.7-18lenny7 GNU C Library: Shared libraries ii libmilter1.0.1 8.14.3-5+lenny1 Sendmail Mail Filter API (Milter) Versions of packages amavisd-milter recommends: ii postfix 2.5.5-1.1 High-performance mail transport ag amavisd-milter suggests no packages. -- no debconf information -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
Dear Gabor Kiss, first thanks for your bug report, it's definitly a bug in the init script although the security implications may only be serious when avamisd-new or amavisd-milter themselves expose a vulnerability as /etc/default/amavisd-milter is owned/writeable only by root and running the init script requires root privileges. Nonetheless this is an issue so I attached a patch which should solve it. Could you please save the attached patch to /tmp/ and apply it by issuing cd /; patch /tmp/amavisd-milter_owner-fix.patch and afterwards tell me if this solved the problem? Kind regards Harald Jenny --- /etc/init.d/amavisd-milter 2010-05-12 23:01:42.0 +0200 +++ /etc/init.d/amavisd-milter_FIXED 2011-01-12 22:25:03.0 +0100 @@ -42,7 +42,7 @@ [ $EXTRAPARAMS ] OPTIONS=$OPTIONS $EXTRAPARAMS [ $PIDFILE ] ([ -d $(dirname $PIDFILE) ] || mkdir $(dirname $PIDFILE) chown $USER:$(id $USER -g -n) $(dirname $PIDFILE)) -[ $MILTERSOCKET ] ([ -d $(dirname $MILTERSOCKET) ] || mkdir $(dirname $MILTERSOCKET) chown $USER $(dirname $MILTERSOCKET)) +([ $MILTERSOCKET ] [ `echo $MILTERSOCKET | grep -v ^inet` ]) ([ -d $(dirname $MILTERSOCKET) ] || mkdir $(dirname $MILTERSOCKET) chown $USER $(dirname $MILTERSOCKET)) START=--start --quiet --chuid $USER --pidfile $PIDFILE --startas $DAEMON --name $NAME -- $OPTIONS STOP=--stop --quiet --retry 10 --pidfile $PIDFILE --startas $DAEMON --name $NAME @@ -56,8 +56,8 @@ start-stop-daemon $START case $? in 0) log_end_msg 0 - [ $MILTERSOCKET ] [ $MILTERSOCKETOWNER ] chown $MILTERSOCKETOWNER $MILTERSOCKET - [ $MILTERSOCKET ] [ $MILTERSOCKETMODE ] chmod $MILTERSOCKETMODE $MILTERSOCKET ;; + [ $MILTERSOCKET ] [ `echo $MILTERSOCKET | grep -v ^inet` ] [ $MILTERSOCKETOWNER ] chown $MILTERSOCKETOWNER $MILTERSOCKET + [ $MILTERSOCKET ] [ `echo $MILTERSOCKET | grep -v ^inet` ] [ $MILTERSOCKETMODE ] chmod $MILTERSOCKETMODE $MILTERSOCKET ;; 1) log_progress_msg already started log_end_msg 0 ;; *) log_end_msg $? ;;
Bug#609762: amavisd-milter: Init script changes owner of current directory to 'amavis'
privileges. Nonetheless this is an issue so I attached a patch which should solve it. Could you please save the attached patch to /tmp/ and apply it by issuing cd /; patch /tmp/amavisd-milter_owner-fix.patch and afterwards tell me if this solved the problem? Dear Jenny, Unfortunately the answer is definitely not. Actually I planned to use amavisd-milter a year ago but later I found it unnecessary. Now I totally removed it. :-( Regards Gabor -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
