Opened 2 years ago

Closed 18 months ago

Last modified 17 months ago

#22594 closed defect (fixed)

Escape characters in contact lines break hourly updater

Reported by: karsten Owned by: iwakeh
Priority: High Milestone: Onionoo 1.12.0
Component: Metrics/Onionoo Version:
Severity: Normal Keywords: metrics-2018
Cc: catalyst Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Onionoo's hourly updater broke on June 12 at around 13:30 UTC. Turns out it couldn't process the following server descriptor (look out for the contact line):

@type server-descriptor 1.0
router HarukaMiddle 46.101.62.152 9001 0 0
identity-ed25519
-----BEGIN ED25519 CERT-----
AQQABlsyAa97mM9YlSM9a8RHbgNwUduV8zMYrUs/uXhfk3fg7ZPEAQAgBAA/B+AS
R+3S5M09GNQjE9EzvGR/FS6s+WjMs9bNdxTideK2fjKqU3mR+QqCvOP7DYEh8/2w
VMChyxEjyKWBRo4iFyVTICqeuStIRLqPAVY/ODcvHbQNbzOY1F8OZSEWWwc=
-----END ED25519 CERT-----
master-key-ed25519 PwfgEkft0uTNPRjUIxPRM7xkfxUurPlozLPWzXcU4nU
platform Tor 0.2.9.11 on Linux
proto Cons=1-2 Desc=1-2 DirCache=1 HSDir=1 HSIntro=3 HSRend=1-2 Link=1-4 LinkAuth=1 Microdesc=1-2 Relay=1-2
published 2017-06-12 12:39:39
fingerprint EABD 6C28 2F28 C6F3 EBB1 AA59 3B50 2071 3B33 D131
uptime 0
bandwidth 2048000 2048000 2220032
extra-info-digest 290AF585354448C448A748CBC632D933CD2880D6 s5rHOgcE+7SAeDUUiQbb9OaYgpfPFOBgW/uJ4SVuXuk
onion-key
-----BEGIN RSA PUBLIC KEY-----
MIGJAoGBAMaI+nbAiMpcMZRXhV6ai9ccHwhp9mr5nNwYunUzdNSyCIB0N56ODZ3c
xme3mG1QQ08um1ewXb6vMxxsiZzzYVxdyawOa+oHMYQEISWyZlvPw+7PDtAg5bGR
8gWqUAIXSTe3K1YXnaXpNcEiwVOO35jDih5HkzkVCvnzz8BCYDUHAgMBAAE=
-----END RSA PUBLIC KEY-----
signing-key
-----BEGIN RSA PUBLIC KEY-----
MIGJAoGBAMygGYMk7d+pYQu5JyCROtRS2LtURSSgfaAGsPUblTGD+ZoSlDDGIu2Q
AUC7QzQxO1HxbUqkoQwo0Im3jsQPcHznIuKevJEzMCPkErn1DIwOidYdW9tTLgcG
4+q5pvwpyd0uDTV2Z8SqXnIyVfAFpAGt8LOOFahJiTMHRRI8ABsFAgMBAAE=
-----END RSA PUBLIC KEY-----
onion-key-crosscert
-----BEGIN CROSSCERT-----
HAn8AxeqoyWnI0h/JhnKoEEs6Lgz/JwmhruEjOC488bOF5G1HJrkM675Sj4B8A3Z
5pGPn0hi1Owbza72nlUtxPuK8pa6ItP/A8Q1v9AvwYhIkmEvHVaCH/FzthW+8AzG
lGiH6FcP/VQ3htQ2kpNUyMrkqLVgDHmVXgT/bXBGsto=
-----END CROSSCERT-----
ntor-onion-key-crosscert 1
-----BEGIN ED25519 CERT-----
AQoABllNAT8H4BJH7dLkzT0Y1CMT0TO8ZH8VLqz5aMyz1s13FOJ1ADVfDOXFoxvI
Zre9gmhKEuPq10ioGbGaKtvj/YrdVsFwNc76mGRSpaM2Ar2DBkxpZBh/p3Cwqe0V
Bbcre5sQOgo=
-----END ED25519 CERT-----
hidden-service-dir
contact Haruka iampsychopath/\@/\live/\./\co/\./\uk
ntor-onion-key 98H8kDWwomC0z+goDIgI2MH0fMkW1I1vbme2dBoo8TY=
reject *:*
router-sig-ed25519 UFYUr1vTeiRwA4grYD/LGLHC8xuzKMSNDijHIcZkckqXdxiXLwRasQagyXuKxwKS+q6A7uIxRUcwdjxq7t1sBA
router-signature
-----BEGIN SIGNATURE-----
wngC1BBTQUrSkZejdr9pYEGJmAreph8x0g0UvA5jjTX7do/ltRrmPN6VSgxbd36y
nHEe+cL8jYXUyLBENxnD4cA4pVxYgqFYWlhgtrDIonmeMWGXfirJBHIbZG/zKeVv
EXRdkh13OHEtUWU1PEGRSNNt7oSQf6rl//Qwz3Andx8=
-----END SIGNATURE-----

Here's the exception:

java.lang.NumberFormatException: \uk","
        at com.google.gson.stream.JsonReader.readEscapeCharacter(JsonReader.java:1466) ~[onionoo-4.0-1.2.0-dev.jar:4.0-1.2.0-dev-595f87a]
        at com.google.gson.stream.JsonReader.nextQuotedValue(JsonReader.java:993) ~[onionoo-4.0-1.2.0-dev.jar:4.0-1.2.0-dev-595f87a]
        at com.google.gson.stream.JsonReader.nextString(JsonReader.java:811) ~[onionoo-4.0-1.2.0-dev.jar:4.0-1.2.0-dev-595f87a]
        at com.google.gson.internal.bind.TypeAdapters$13.read(TypeAdapters.java:358) ~[onionoo-4.0-1.2.0-dev.jar:4.0-1.2.0-dev-595f87a]
        at com.google.gson.internal.bind.TypeAdapters$13.read(TypeAdapters.java:346) ~[onionoo-4.0-1.2.0-dev.jar:4.0-1.2.0-dev-595f87a]
        at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory$1.read(ReflectiveTypeAdapterFactory.java:93) ~[onionoo-4.0-1.2.0-dev.jar:4.0-1.2.0-dev-595f87a]
        at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory$Adapter.read(ReflectiveTypeAdapterFactory.java:172) ~[onionoo-4.0-1.2.0-dev.jar:4.0-1.2.0-dev-595f87a]
        at com.google.gson.Gson.fromJson(Gson.java:803) ~[onionoo-4.0-1.2.0-dev.jar:4.0-1.2.0-dev-595f87a]
        at com.google.gson.Gson.fromJson(Gson.java:768) ~[onionoo-4.0-1.2.0-dev.jar:4.0-1.2.0-dev-595f87a]
        at com.google.gson.Gson.fromJson(Gson.java:717) ~[onionoo-4.0-1.2.0-dev.jar:4.0-1.2.0-dev-595f87a]
        at com.google.gson.Gson.fromJson(Gson.java:689) ~[onionoo-4.0-1.2.0-dev.jar:4.0-1.2.0-dev-595f87a]
        at org.torproject.onionoo.docs.DocumentStore.retrieveParsedDocumentFile(DocumentStore.java:539) [onionoo-4.0-1.2.0-dev.jar:4.0-1.2.0-dev-595f87a]
        at org.torproject.onionoo.docs.DocumentStore.retrieveDocumentFile(DocumentStore.java:505) [onionoo-4.0-1.2.0-dev.jar:4.0-1.2.0-dev-595f87a]
        at org.torproject.onionoo.docs.DocumentStore.retrieve(DocumentStore.java:378) [onionoo-4.0-1.2.0-dev.jar:4.0-1.2.0-dev-595f87a]
        at org.torproject.onionoo.updater.NodeDetailsStatusUpdater.processRelayServerDescriptor(NodeDetailsStatusUpdater.java:151) [onionoo-4.0-1.2.0-dev.jar:4.0-1.2.0-dev-595f87a]
        at org.torproject.onionoo.updater.NodeDetailsStatusUpdater.processDescriptor(NodeDetailsStatusUpdater.java:130) [onionoo-4.0-1.2.0-dev.jar:4.0-1.2.0-dev-595f87a]
        at org.torproject.onionoo.updater.DescriptorSource.readDescriptors(DescriptorSource.java:132) [onionoo-4.0-1.2.0-dev.jar:4.0-1.2.0-dev-595f87a]
        at org.torproject.onionoo.updater.DescriptorSource.readDescriptors(DescriptorSource.java:97) [onionoo-4.0-1.2.0-dev.jar:4.0-1.2.0-dev-595f87a]
        at org.torproject.onionoo.cron.Main.updateStatuses(Main.java:180) [onionoo-4.0-1.2.0-dev.jar:4.0-1.2.0-dev-595f87a]
        at org.torproject.onionoo.cron.Main.run(Main.java:129) [onionoo-4.0-1.2.0-dev.jar:4.0-1.2.0-dev-595f87a]
        at org.torproject.onionoo.cron.Main.runOrScheduleExecutions(Main.java:103) [onionoo-4.0-1.2.0-dev.jar:4.0-1.2.0-dev-595f87a]
        at org.torproject.onionoo.cron.Main.main(Main.java:35) [onionoo-4.0-1.2.0-dev.jar:4.0-1.2.0-dev-595f87a]

This issue was quite well hidden, because Gson.fromJson apparently doesn't catch NumberFormatException, so the main thread just dies.

Here's the (pretty-printed) string that it attempted to parse:

{
  "desc_published": "2017-06-12 12:39:39",
  "last_restarted": "2017-06-12 12:39:39",
  "bandwidth_rate": 2048000,
  "bandwidth_burst": 2048000,
  "observed_bandwidth": 2220032,
  "advertised_bandwidth": 2048000,
  "exit_policy": [
    "reject *:*"
  ],
  "contact":"Haruka iampsychopath/\\\\@/\\\\live/\\\\./\\\\co/\\\\./\\\uk",
  "platform": "Tor 0.2.9.11 on Linux",
  "alleged_family": [],
  "effective_family": [],
  "indirect_family": [],
  "is_relay": true,
  "running": true,
  "nickname": "HarukaMiddle",
  "address": "46.101.62.152",
  "or_addresses_and_ports": [],
  "first_seen_millis": 1497031200000,
  "last_seen_millis": 1497268800000,
  "or_port": 9001,
  "dir_port": 0,
  "relay_flags": [
    "Fast",
    "Running",
    "Valid"
  ],
  "consensus_weight": 483,
  "default_policy": "reject",
  "port_list": "1-65535",
  "last_changed_or_address_or_port": 1497031200000,
  "recommended_version": true,
  "measured": true,
  "exit_addresses": {},
  "consensus_weight_fraction": 1.1896908e-05,
  "guard_probability": 0,
  "middle_probability": 3.122049e-05,
  "exit_probability": 0,
  "latitude": 51.5092,
  "longitude": -0.0955,
  "country_code": "gb",
  "country_name": "United Kingdom",
  "region_name": "England",
  "city_name": "London",
  "as_name": "Digital Ocean, Inc.",
  "as_number": "AS14061",
  "host_name": "46.101.62.152"
}

I deployed the following hotfix:

diff --git a/src/main/java/org/torproject/onionoo/docs/DocumentStore.java b/src/main/java/org/torproject/onionoo/docs/DocumentStore.java
index 34bc8ef..246c02b 100644
--- a/src/main/java/org/torproject/onionoo/docs/DocumentStore.java
+++ b/src/main/java/org/torproject/onionoo/docs/DocumentStore.java
@@ -537,8 +537,9 @@ public class DocumentStore {
     Gson gson = new Gson();
     try {
       result = gson.fromJson(documentString, documentType);
-    } catch (JsonParseException e) {
+    } catch (JsonParseException | NumberFormatException e) {
       /* Handle below. */
+      log.error(documentString);
       log.error(e.getMessage(), e);
     }
     if (result == null) {

The hourly update is still running, but I believe the result will be that the relay publishing this descriptor will simply not show up in Onionoo results, or at least not with recent data. Should be fine for the moment, but deserves a better fix. The real fix is to check our logic for escaping/unescaping JSON strings, yet once more.

Child Tickets

Attachments (1)

2017-06-12-13-05-00-server-descriptors (1.6 MB) - added by karsten 2 years ago.
CollecTor's server descriptors file containing problematic descriptor

Download all attachments as: .zip

Change History (20)

Changed 2 years ago by karsten

CollecTor's server descriptors file containing problematic descriptor

comment:1 Changed 2 years ago by iwakeh

Regarding the hot-fix I would suggest to catch any Throwable in the above patch in order to also have possible future Gson exceptions logged.

Maybe, a switch to a fully JSON compliant library would also help, but that and possible other solutions need more time, that's true.

comment:2 Changed 2 years ago by catalyst

Cc: catalyst added

comment:3 in reply to:  1 Changed 2 years ago by karsten

Replying to iwakeh:

Regarding the hot-fix I would suggest to catch any Throwable in the above patch in order to also have possible future Gson exceptions logged.

Maybe, a switch to a fully JSON compliant library would also help, but that and possible other solutions need more time, that's true.

I just pushed the hotfix to master, which now catches Throwable.

comment:4 Changed 2 years ago by iwakeh

Why two log statements?

 catch (Throwable e) {
       /* Handle below. */
+      log.error("Problem while parsing JSON: " + documentString, e);
-      log.error(documentString);
-      log.error(e.getMessage(), e);
 }

comment:5 Changed 2 years ago by karsten

No specific reason, other than being in a rush to put out a hotfix. Happy to undo that when we put in a real fix. :)

comment:6 Changed 2 years ago by karsten

Keywords: metrics-2018 added

comment:7 Changed 22 months ago by iwakeh

Priority: HighMedium

This was part of release onionoo-4.0-1.3.0 already.
Setting priority to 'medium'.

comment:8 Changed 22 months ago by karsten

Did we really fix the bug or just deploy a hotfix back then?

comment:9 Changed 22 months ago by iwakeh

The hotfix was added to master and did go into the onionoo-4.0-1.3.0 release onward.
I didn't close this ticket b/c it doesn't seem there was a real fix yet.
This together with having tag 'metrics-2018' the priority ought to be rather medium here.

Last edited 22 months ago by iwakeh (previous) (diff)

comment:10 Changed 22 months ago by karsten

Yep, sounds good.

comment:11 Changed 21 months ago by karsten

Priority: MediumHigh

Raising priority to prevent bad operators from "hiding" their relays from Onionoo clients. See this thread for more context.

comment:12 Changed 19 months ago by iwakeh

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

comment:13 Changed 18 months ago by iwakeh

Status: acceptedneeds_review

Please review this patch, which avoid the above troubles, i.e., NFE, by only un-escaping valid utf.

comment:14 Changed 18 months ago by karsten

Status: needs_reviewneeds_revision

Looks good! Two things:

  • Can you add a change log entry? Maybe something like this: Don't attempt to un-escape character sequences in contact lines (like "\uk") that only happen to start like escaped utf-8 characters (like "\u0055").
  • There's similar code where we replace "\\\\u" with "\\u" in ResponseBuilder. Does it make sense to use the new method there, too?

Thanks!

comment:15 in reply to:  14 Changed 18 months ago by iwakeh

Status: needs_revisionaccepted

Replying to karsten:

Looks good! Two things:

  • Can you add a change log entry? Maybe something like this: Don't attempt to un-escape character sequences in contact lines (like "\uk") that only happen to start like escaped utf-8 characters (like "\u0055").

Yep, that was missing.

  • There's similar code where we replace "\\\\u" with "\\u" in ResponseBuilder. Does it make sense to use the new method there, too?

Sure. I was actually intending to add a new ticket about having all un/escape logic as well as the json-reading and writing in one place, which would make it easier to switch to some other JSON lib than gson in future and have the logic separate for re-processing files etc.
But, it's fine to start with putting all un/escape logic in one place and add the remaining to a new ticket in case it would be too much for one ticket.

Thanks!

comment:16 Changed 18 months ago by iwakeh

Status: acceptedneeds_review

Please find two more commits for review.

comment:17 Changed 18 months ago by karsten

Resolution: fixed
Status: needs_reviewclosed

Looks good! Merged. Closing. Thanks!

comment:18 Changed 18 months ago by cypherpunks

regression: #25740

comment:19 Changed 17 months ago by iwakeh

Milestone: Onionoo 1.12.0
Note: See TracTickets for help on using tickets.