Code review comment for lp:~ksa618/ubuntu/oneiric/apt/fix-for-404724

On Sun, Jun 26, 2011 at 03:18:42PM -0000, Kenneth Solbø Andersen wrote:
> Kenneth Solbø Andersen has proposed merging lp:~ksa618/ubuntu/oneiric/apt/fix-for-404724 into lp:ubuntu/apt.
>
> Requested reviews:
> Michael Vogt (mvo)
> Related bugs:
> Bug #404724 in apt (Ubuntu): "unnecessarily restrictive permissions on /var/log/apt/term.log"
> https://bugs.launchpad.net/ubuntu/+source/apt/+bug/404724
>
> For more details, see:
> https://code.launchpad.net/~ksa618/ubuntu/oneiric/apt/fix-for-404724/+merge/65900
>
> Sets term.log permissions to root.adm and 644

Thanks a bunch for your patch! I'm traveling right now so I can not
commit it right away. But from a quick look it looks great! The only
small thing that should be checked is if any of the calls return
NULL. While highly unlikely its still safer as otherwise the null
pointer dereference will cause a segfault.

Thanks,
 Michael

> --
> https://code.launchpad.net/~ksa618/ubuntu/oneiric/apt/fix-for-404724/+merge/65900
> You are requested to review the proposed merge of lp:~ksa618/ubuntu/oneiric/apt/fix-for-404724 into lp:ubuntu/apt.

> === modified file 'apt-pkg/deb/dpkgpm.cc'
> --- apt-pkg/deb/dpkgpm.cc 2011-06-07 15:55:07 +0000
> +++ apt-pkg/deb/dpkgpm.cc 2011-06-26 15:18:34 +0000
> @@ -32,6 +32,8 @@
> #include <algorithm>
> #include <sstream>
> #include <map>
> +#include <pwd.h>
> +#include <grp.h>
>
> #include <termios.h>
> #include <unistd.h>
> @@ -666,7 +668,12 @@
> return _error->WarningE("OpenLog", _("Could not open file '%s'"), logfile_name.c_str());
> setvbuf(term_out, NULL, _IONBF, 0);
> SetCloseExec(fileno(term_out), true);
> - chmod(logfile_name.c_str(), 0600);
> + struct passwd *pw;
> + struct group *gr;
> + pw = getpwnam("root");
> + gr = getgrnam("adm");
> + chown(logfile_name.c_str(), pw->pw_uid, gr->gr_gid);
> + chmod(logfile_name.c_str(), 0644);
> fprintf(term_out, "\nLog started: %s\n", timestr);
> }
>
>
> === modified file 'debian/changelog'
> --- debian/changelog 2011-06-09 13:56:25 +0000
> +++ debian/changelog 2011-06-26 15:18:34 +0000
> @@ -1,3 +1,10 @@
> +apt (0.8.14.1ubuntu8) oneiric; urgency=low
> +
> + * apt-pkg/deb/dpkgpm.cc:
> + - set permissions of term.log to root.adm and 644 (LP: #404724)
> +
> + -- Kenneth Solb?? Andersen <email address hidden> Sun, 26 Jun 2011 15:36:56 +0200
> +
> apt (0.8.14.1ubuntu7) oneiric; urgency=low
>
> [ Michael Vogt ]
>