fix tcpdump localtime caching

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

fix tcpdump localtime caching

Holger Mikolon
The comment above priv_localtime() says, the obtained localtime (from the
privileged process) is cached for about one minute. However, since the
according if statement compares the wrong variable, the caching doesn't
happen. This bug is there since the very first file version (from 15+
years ago).

Regards
Holger


Index: usr.sbin/tcpdump/privsep.c
===================================================================
RCS file: /cvs/src/usr.sbin/tcpdump/privsep.c,v
retrieving revision 1.53
diff -u -p -u -r1.53 privsep.c
--- usr.sbin/tcpdump/privsep.c 18 Mar 2019 00:09:22 -0000 1.53
+++ usr.sbin/tcpdump/privsep.c 10 May 2019 13:17:42 -0000
@@ -727,7 +727,7 @@ priv_localtime(const time_t *t)
  static struct tm *gt = NULL;
  static char zone[PATH_MAX];
 
- if (gt != NULL) {
+ if (t != NULL) {
  gt = gmtime(t);
  gt0.tm_sec = gt->tm_sec;
  gt0.tm_zone = gt->tm_zone;

Reply | Threaded
Open this post in threaded view
|

Re: fix tcpdump localtime caching

Jeremie Courreges-Anglas-2
On Fri, May 10 2019, Holger Mikolon <[hidden email]> wrote:
> The comment above priv_localtime() says, the obtained localtime (from the
> privileged process) is cached for about one minute. However, since the
> according if statement compares the wrong variable, the caching doesn't
> happen. This bug is there since the very first file version (from 15+
> years ago).

The variables could have more meaningful names, also the reuse of
variable "gt" looks hackish, but the current code looks correct to me.

Does the diff below make things clearer?


--- privsep.c.~1.53.~ Sat May 11 14:17:40 2019
+++ privsep.c Sat May 11 14:20:30 2019
@@ -724,10 +724,12 @@ struct tm *
 priv_localtime(const time_t *t)
 {
  static struct tm lt, gt0;
- static struct tm *gt = NULL;
  static char zone[PATH_MAX];
+ static int cached = 0;
 
- if (gt != NULL) {
+ if (cached) {
+ struct tm *gt;
+
  gt = gmtime(t);
  gt0.tm_sec = gt->tm_sec;
  gt0.tm_zone = gt->tm_zone;
@@ -749,7 +751,7 @@ priv_localtime(const time_t *t)
  lt.tm_zone = NULL;
 
  gt0.tm_zone = NULL;
- gt = &gt0;
+ cached = 1;
 
  return &lt;
 }

--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Reply | Threaded
Open this post in threaded view
|

Re: fix tcpdump localtime caching

Otto Moerbeek
In reply to this post by Holger Mikolon
On Fri, May 10, 2019 at 03:25:17PM +0200, Holger Mikolon wrote:

> The comment above priv_localtime() says, the obtained localtime (from the
> privileged process) is cached for about one minute. However, since the
> according if statement compares the wrong variable, the caching doesn't
> happen. This bug is there since the very first file version (from 15+
> years ago).
>
> Regards
> Holger

No, this is not correct.

The point is that the code remembers the timezone corrected value so
it does not have to consult the privileged process for it all the time.

Once the timezone corrected date and hour and mintue are known, only
seconds are updated via a call to gmtime().

        -Otto

>
>
> Index: usr.sbin/tcpdump/privsep.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/tcpdump/privsep.c,v
> retrieving revision 1.53
> diff -u -p -u -r1.53 privsep.c
> --- usr.sbin/tcpdump/privsep.c 18 Mar 2019 00:09:22 -0000 1.53
> +++ usr.sbin/tcpdump/privsep.c 10 May 2019 13:17:42 -0000
> @@ -727,7 +727,7 @@ priv_localtime(const time_t *t)
>   static struct tm *gt = NULL;
>   static char zone[PATH_MAX];
>  
> - if (gt != NULL) {
> + if (t != NULL) {
>   gt = gmtime(t);
>   gt0.tm_sec = gt->tm_sec;
>   gt0.tm_zone = gt->tm_zone;
>

Reply | Threaded
Open this post in threaded view
|

Re: fix tcpdump localtime caching

Holger Mikolon
In reply to this post by Jeremie Courreges-Anglas-2
> The variables could have more meaningful names, also the reuse of
> variable "gt" looks hackish, but the current code looks correct to me.
>
> Does the diff below make things clearer?

It does. After reading the current code again a few times, it is
as well clear. Incredible how I couldn't see it before.
My apologies! And many thanks for the helpful replies to you and Otto!

Holger

>
> --- privsep.c.~1.53.~ Sat May 11 14:17:40 2019
> +++ privsep.c Sat May 11 14:20:30 2019
> @@ -724,10 +724,12 @@ struct tm *
>  priv_localtime(const time_t *t)
>  {
>   static struct tm lt, gt0;
> - static struct tm *gt = NULL;
>   static char zone[PATH_MAX];
> + static int cached = 0;
>  
> - if (gt != NULL) {
> + if (cached) {
> + struct tm *gt;
> +
>   gt = gmtime(t);
>   gt0.tm_sec = gt->tm_sec;
>   gt0.tm_zone = gt->tm_zone;
> @@ -749,7 +751,7 @@ priv_localtime(const time_t *t)
>   lt.tm_zone = NULL;
>  
>   gt0.tm_zone = NULL;
> - gt = &gt0;
> + cached = 1;
>  
>   return &lt;
>  }
>
> --
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE