Merge lp:~louis/ubuntu/trusty/clamav/lp799623_fix_logrotate into lp:ubuntu/trusty/clamav

Proposed by Louis Bouchard on 2014-02-10
Status: Merged
Merge reported by: Martin Pitt
Merged at revision: not available
Proposed branch: lp:~louis/ubuntu/trusty/clamav/lp799623_fix_logrotate
Merge into: lp:ubuntu/trusty/clamav
Diff against target: 28 lines (+9/-1)
2 files modified
debian/changelog (+8/-0)
debian/common_functions (+1/-1)
To merge this branch: bzr merge lp:~louis/ubuntu/trusty/clamav/lp799623_fix_logrotate
Reviewer Review Type Date Requested Status
Martin Pitt 2014-02-10 Approve on 2014-02-12
Review via email: mp+205627@code.launchpad.net
To post a comment you must log in.
Martin Pitt (pitti) wrote :

This looks like it could easily introduce a regression, especially on config files which look "normal", i. e. only use one space to separate words. Are you sure that instead of

  value=`grep ^$variable[[:space:]] $CLAMAVCONF | head -n1 | awk '{print $2}'`

you don't mean

  value=`grep ^$variable[[:space:]]* $CLAMAVCONF | head -n1 | awk '{print $2}'`

?

review: Needs Fixing
Martin Pitt (pitti) wrote :

Oh, the $CLAMAVCONF is not part of the search string indeed. This really ought to be quoted, like

value=`grep "^$variable " $CLAMAVCONF | head -n1 | awk '{print $2}'`

Martin Pitt (pitti) :
review: Approve
Colin Watson (cjwatson) wrote :

On Wed, Feb 12, 2014 at 03:12:52PM -0000, Martin Pitt wrote:
> value=`grep ^$variable[[:space:]]* $CLAMAVCONF | head -n1 | awk '{print $2}'`

Also surely that regex should be double-quoted? Otherwise that might
happen to expand to something in the current working directory and weird
stuff would ensue.

Louis Bouchard (louis) wrote :

Makes perfect sense to me. I was too trusty of the patch proposed in the bug.

I will rework according to the suggestions

Martin Pitt (pitti) wrote :

Uploaded with

- value=`grep ^$variable $CLAMAVCONF | head -n1 | awk '{print $2}'`
+ value=`grep "^$variable[[:space:]]" $CLAMAVCONF | head -n1 | awk '{print $2}'`

now. I didn't actually use UDD, that branch takes abysmally long to check out (I killed it after 10 mins or so).

Thanks!

Louis Bouchard (louis) wrote :

Before I go any further, the addition of [[:space:]] was to discriminate on word boundary. When the CLAMAVCONF file has entries like this :

StreamMaxLength 25M
LogFileMaxSize 0
LogFile /var/log/clamav/clamav.log

the grep could not discriminate b/w LogFileMaxSize & LogFile, so the LogFile variable ended up with 0.

Revisiting the expression, I think that using \b would be more appropriate to discriminate on word boudaries so the expression would be :

value=`grep "^$variable\b" $CLAMAVCONF | head -n1 | awk '{print $2}'`

Unless you see an issue with this, I will go ahead with that fix, since the [[:space:]] was not intended to identify single space, but word boundaries

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/changelog'
2--- debian/changelog 2014-02-03 17:04:49 +0000
3+++ debian/changelog 2014-02-10 17:01:04 +0000
4@@ -1,3 +1,11 @@
5+clamav (0.98.1+dfsg-1ubuntu5) trusty; urgency=low
6+
7+ * debian/common_functions
8+ - Fix slurp_config() that is sometimes not discriminative
9+ enough which leads to invalid configuration files (LP: #799623)
10+
11+ -- Louis Bouchard <louis.bouchard@ubuntu.com> Mon, 10 Feb 2014 17:56:52 +0100
12+
13 clamav (0.98.1+dfsg-1ubuntu4) trusty; urgency=medium
14
15 * Stop using a cargo-culted syscall table and trust the glibc headers.
16
17=== modified file 'debian/common_functions'
18--- debian/common_functions 2012-06-19 21:39:28 +0000
19+++ debian/common_functions 2014-02-10 17:01:04 +0000
20@@ -147,7 +147,7 @@
21 value=`grep ^$variable $CLAMAVCONF | head -n1 | sed -e s/$variable\ //`
22 ;;
23 *)
24- value=`grep ^$variable $CLAMAVCONF | head -n1 | awk '{print $2}'`
25+ value=`grep ^$variable[[:space:]] $CLAMAVCONF | head -n1 | awk '{print $2}'`
26 ;;
27 esac
28 if [ -z "$value" ]; then

Subscribers

People subscribed via source and target branches