Merge lp:~ksa618/ubuntu/oneiric/apt/fix-for-404724 into lp:ubuntu/oneiric/apt

[Kenneth Solbø Andersen]

Sets term.log permissions to root.adm and 644

[Michael Vogt (mvo)]

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

[Kenneth Solbø Andersen (ksa618)]

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

[Michael Vogt (mvo)]

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

[lp:~ksa618/ubuntu/oneiric/apt/fix-for-404724]

149. By Kenneth Solbø Andersen on 2011-06-26

add check for NULL in getpwnam/getgrnam

[Michael Vogt (mvo)]

Thanks! This looks great, I merged it into my "mvo" branch and it will go into both debian and ubuntu with the next upload.

review: Approve

=== 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 16:26:31 +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,13 @@
 	 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");
+      if (pw != NULL && gr != NULL)
+	  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 16:26:31 +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 <[e-mail]>  Sun, 26 Jun 2011 15:36:56 +0200
+
 apt (0.8.14.1ubuntu7) oneiric; urgency=low
 
   [ Michael Vogt ]