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

On Sun, Jun 26, 2011 at 04:07:26PM -0000, Kenneth Solbø Andersen wrote:
> 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?

Thanks, there are no really strict rules for this, but my advice is to
leave the debian/changelog as it is and just use like:
"bzr commit -m 'add check for NULL in getpwnam/getgrnam'"

and then "bzr push" so that it appears on the server. Unless of course
you already used "bzr checkout" in which case the push will happen
automatically.

Thanks,
 Michael

> - 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.
> >
>
> --
> 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.