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

On Sun, Jun 26, 2011 at 5:52 PM, Michael Vogt <email address hidden>wrote:

> 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<https://code.launchpad.net/%7Eksa618/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
>

I've added a check for NULL locally, but, since I'm new to bug fixing, I'm
unsure about what to do now. Do I update the changelog and use debcommit or
leave the changelog unaltered and use debcommit, or something else?

- Kenneth

>
> > --
> >
> https://code.launchpad.net/~ksa618/ubuntu/oneiric/apt/fix-for-404724/+merge/65900<https://code.launchpad.net/%7Eksa618/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 ]
> >
>
>
> --
>
> https://code.launchpad.net/~ksa618/ubuntu/oneiric/apt/fix-for-404724/+merge/65900<https://code.launchpad.net/%7Eksa618/ubuntu/oneiric/apt/fix-for-404724/+merge/65900>
> You are the owner of lp:~ksa618/ubuntu/oneiric/apt/fix-for-404724.
>