Opened 22 months ago

Closed 6 days ago

Last modified 5 days ago

#20224 closed defect (fixed)

Fix `BridgeDescriptorMappingsLimit` config option

Reported by: karsten Owned by: karsten
Priority: Low Milestone: CollecTor 1.7.0
Component: Metrics/CollecTor Version:
Severity: Normal Keywords: metrics-2018
Cc: iwakeh, metrics-team Actual Points:
Parent ID: Points:
Reviewer: irl Sponsor:

Description

The BridgeDescriptorMappingsLimit option doesn't work as expected with its default value inf or with other sufficiently large values. Here's where that configuration value is used (https://gitweb.torproject.org/collector.git/tree/src/main/java/org/torproject/collector/bridgedescs/SanitizedBridgesWriter.java#n183):

      this.bridgeSanitizingCutOffTimestamp = formatter.format(
          System.currentTimeMillis() - 24L * 60L * 60L * 1000L
          * limitBridgeSanitizingInterval);

For limitBridgeSanitizingInterval = Integer.MAX_VALUE, we'd try to format 1474617184667 - 86400000 * 2147483647 = -1.855E+017 milliseconds since the epoch. In theory, that large negative value should still fit into the long, but somehow it gets formatted as 5877475-11-25 07:53:04, which is in the future rather than in the past.

Anyway, I stopped digging what the exact bug might be, and instead suggest this possible fix:

diff --git a/src/main/java/org/torproject/collector/bridgedescs/SanitizedBridgesWriter.java b/src/main/java/org/torproject/collector/bridgedescs/SanitizedBridgesWriter.java
index b61cd30..b8674a3 100644
--- a/src/main/java/org/torproject/collector/bridgedescs/SanitizedBridgesWriter.java
+++ b/src/main/java/org/torproject/collector/bridgedescs/SanitizedBridgesWriter.java
@@ -180,9 +180,9 @@ public class SanitizedBridgesWriter extends CollecTorMain {
       SimpleDateFormat formatter = new SimpleDateFormat(
           "yyyy-MM-dd HH:mm:ss");
       formatter.setTimeZone(TimeZone.getTimeZone("UTC"));
-      this.bridgeSanitizingCutOffTimestamp = formatter.format(
+      this.bridgeSanitizingCutOffTimestamp = formatter.format(Math.max(-1L,
           System.currentTimeMillis() - 24L * 60L * 60L * 1000L
-          * limitBridgeSanitizingInterval);
+          * limitBridgeSanitizingInterval));
     } else {
       this.bridgeSanitizingCutOffTimestamp = "1999-12-31 23:59:59";
     }

Child Tickets

Change History (18)

comment:1 Changed 22 months ago by karsten

Status: newneeds_review

Hmm, looks like I should have set this to needs_review before. Doing that now.

comment:2 Changed 22 months ago by iwakeh

I'd rather set the time explicitly in case of a too small value, i.e.

       SimpleDateFormat formatter = new SimpleDateFormat(
           "yyyy-MM-dd HH:mm:ss");
       formatter.setTimeZone(TimeZone.getTimeZone("UTC"));
       long cutTime = System.currentTimeMillis() - 24L * 60L * 60L * 1000L
           * limitBridgeSanitizingInterval);

       if (cutTime > -Integer.MAX_VALUE) 
         this.bridgeSanitizingCutOffTimestamp = formatter.format(cutTime);
       } else {
         this.bridgeSanitizingCutOffTimestamp = "1999-12-31 23:59:59";
       }

(Assuming 1999-12-31 23:59:59 is the smallest possible time stamp that makes sense.)

comment:3 Changed 22 months ago by karsten

Hmm, no, that seems wrong or at least confusing, and on second thought my patch above is wrong or at least confusing, too.

Here's a table with some timestamps that seem to be relevant here:

Row BridgeDescriptorMappingsLimit Milliseconds since the epochISO-8601 date/time
1 0 1475517323620 AD 2016-10-03 17:55:23.620
2 6120.7468 946684799999 AD 1999-12-31 23:59:59.999
3 17077.7468 -1 AD 1969-12-31 23:59:59.999
4 17102.6019 -Integer.MAX_VALUE = -2147483647 AD 1969-12-07 03:28:36.353
5 -Integer.MAX_VALUE = -2147483647 -185541111583476380 BC 5877475-12-05 17:55:23.620

Explanations for these rows:

  1. This is the current date/time, so if we had put in 0 as value for BridgeDescriptorMappingsLimit, that's what the cut off time would have been.
  2. This is what we had put in before using Integer.MAX_VALUE as parameter default value, and this date in 1999 is indeed small enough that no bridge descriptor could have been published earlier.
  3. This is what I had in mind in my patch above: pick -1 as smallest possible time in milliseconds since the epoch, but only because I had misread 1999 as 1969. Oops.
  4. That's what I think you're suggesting above, and I think it doesn't really make sense to use this threshold to decide whether or not to fall back to a date in 1999.
  5. Here's the explanation for this bug, which I found out today: we're not looking at an integer overflow/underflow, we're looking at BC dates rather than AD dates, and we're not including AD/BC in the output date format. Ugh!

My suggestion is to cut off at 1999-12-31 23:59:59 by hard-coding that date/time in milliseconds as threshold. Untested code:

       SimpleDateFormat formatter = new SimpleDateFormat(
           "yyyy-MM-dd HH:mm:ss");
       formatter.setTimeZone(TimeZone.getTimeZone("UTC"));
       long cutTime = System.currentTimeMillis() - 24L * 60L * 60L * 1000L
           * limitBridgeSanitizingInterval);

       final long MIN_CUT_TIME = 946684799999L;
       if (cutTime > MIN_CUT_TIME) {
         this.bridgeSanitizingCutOffTimestamp = formatter.format(cutTime);
       } else {
         this.bridgeSanitizingCutOffTimestamp = "1999-12-31 23:59:59";
       }

Does this make sense? If so, want to submit another patch?

comment:4 Changed 22 months ago by iwakeh

Owner: set to iwakeh
Status: needs_reviewassigned

Oh, those date formats must have caused many programming/troubleshooting years on earth, in total.

The cut-off makes sense; there are no older descriptors.

I'll add this to my list.

comment:5 Changed 10 months ago by karsten

Keywords: metrics-2018 added

comment:6 Changed 6 months ago by iwakeh

Owner: changed from iwakeh to metrics-team

Move to metrics-team as these are not worked on by me during the next week.

comment:7 Changed 4 months ago by iwakeh

Owner: changed from metrics-team to iwakeh
Status: assignedaccepted

comment:8 Changed 3 months ago by irl

Cc: metrics-team added

Adding metrics-team to cc

comment:9 Changed 3 months ago by iwakeh

Status: acceptedneeds_review

Please review this patch commit.

comment:10 Changed 3 months ago by iwakeh

Milestone: CollecTor 1.6.0

comment:11 Changed 3 months ago by karsten

Status: needs_reviewneeds_revision

Hmm, that doesn't fix the problem here. Sample log output:

2018-04-16 10:30:18,170 INFO o.t.c.b.SanitizedBridgesWriter:212 Using cut-off datetime '+5877594-10-07 10:30:18' for secrets.

And can you also include a change log entry? Thanks!

comment:12 Changed 8 weeks ago by iwakeh

Owner: changed from iwakeh to metrics-team
Status: needs_revisionassigned

Not top priority with the currently planned tight release schedule; returninng to metrics-team.

comment:13 Changed 7 weeks ago by iwakeh

Milestone: CollecTor 1.6.0CollecTor 1.7.0

Moved to new milestone.

comment:14 Changed 11 days ago by karsten

Owner: changed from metrics-team to karsten
Status: assignedaccepted

Taking over from iwakeh here. irl, please review my task-20224-2 branch, which is a rebase of iwakeh's branch linked above plus a squash commit with two fixes.

comment:15 Changed 11 days ago by karsten

Status: acceptedneeds_review

comment:16 Changed 7 days ago by irl

Reviewer: irl

Will start review this week.

comment:17 Changed 7 days ago by irl

Status: needs_reviewmerge_ready

Looks good to me.

comment:18 Changed 6 days ago by karsten

Resolution: fixed
Status: merge_readyclosed

Thanks for looking! Rebased and pushed to master. Closing.

Note: See TracTickets for help on using tickets.