Turasa / libsignal-service-java

GNU General Public License v3.0
42 stars 22 forks source link

DeviceGroupsInputStream is throwing InvalidProtocolBufferExceptions #3

Closed Trolldemorted closed 8 years ago

Trolldemorted commented 8 years ago

First of all, i do not know whether this is my problem, a vanilla libsignal problem, a turasa libsignal problem or a signal-cli problem, but i i think this is the best place to ask.

As you maybe remember, i am working on a PR that implements linking Signal-Android as a slave device. Today i wanted to implement receiving and handling group sync messages, but so far i had no luck.

Here you can see how i handle a group sync message. In my opinion that is equivalent to what signal-cli is doing here, but unfortunately it does not work.

The first group is handled fine (it is parsed correctly and added to signal-android's groupstore, and i can use it), but when calling read a second time i get this exception:

11-24 00:47:46.074 20816-20846/org.thoughtcrime.securesms E/AndroidRuntime: FATAL EXCEPTION: JobConsumer-2
        Process: org.thoughtcrime.securesms, PID: 20816
        Theme: themes:{}
        java.lang.AssertionError: com.google.protobuf.InvalidProtocolBufferException: Protocol message tag had invalid wire type.
            at org.thoughtcrime.securesms.jobs.PushDecryptJob.handleSynchronizeGroupsMessage(PushDecryptJob.java:357)
            at org.thoughtcrime.securesms.jobs.PushDecryptJob.handleMessage(PushDecryptJob.java:174)
            at org.thoughtcrime.securesms.jobs.PushDecryptJob.onRun(PushDecryptJob.java:138)
            at org.whispersystems.jobqueue.JobConsumer.runJob(JobConsumer.java:76)
            at org.whispersystems.jobqueue.JobConsumer.run(JobConsumer.java:46)
         Caused by: com.google.protobuf.InvalidProtocolBufferException: Protocol message tag had invalid wire type.
            at com.google.protobuf.UnknownFieldSet$Builder.mergeFieldFrom(UnknownFieldSet.java:498)
            at com.google.protobuf.GeneratedMessage.parseUnknownField(GeneratedMessage.java:193)
            at org.whispersystems.signalservice.internal.push.SignalServiceProtos$GroupDetails.<init>(SignalServiceProtos.java:11796)
            at org.whispersystems.signalservice.internal.push.SignalServiceProtos$GroupDetails.<init>(SignalServiceProtos.java:11754)
            at org.whispersystems.signalservice.internal.push.SignalServiceProtos$GroupDetails$1.parsePartialFrom(SignalServiceProtos.java:11871)
            at org.whispersystems.signalservice.internal.push.SignalServiceProtos$GroupDetails$1.parsePartialFrom(SignalServiceProtos.java:11866)
            at com.google.protobuf.AbstractParser.parsePartialFrom(AbstractParser.java:141)
            at com.google.protobuf.AbstractParser.parseFrom(AbstractParser.java:176)
            at com.google.protobuf.AbstractParser.parseFrom(AbstractParser.java:188)
            at com.google.protobuf.AbstractParser.parseFrom(AbstractParser.java:193)
            at com.google.protobuf.AbstractParser.parseFrom(AbstractParser.java:49)
            at org.whispersystems.signalservice.internal.push.SignalServiceProtos$GroupDetails.parseFrom(SignalServiceProtos.java:12653)
            at org.whispersystems.signalservice.api.messages.multidevice.DeviceGroupsInputStream.read(DeviceGroupsInputStream.java:32)
            at org.thoughtcrime.securesms.jobs.PushDecryptJob.handleSynchronizeGroupsMessage(PushDecryptJob.java:354)
            at org.thoughtcrime.securesms.jobs.PushDecryptJob.handleMessage(PushDecryptJob.java:174) 
            at org.thoughtcrime.securesms.jobs.PushDecryptJob.onRun(PushDecryptJob.java:138) 
            at org.whispersystems.jobqueue.JobConsumer.runJob(JobConsumer.java:76) 
            at org.whispersystems.jobqueue.JobConsumer.run(JobConsumer.java:46) 

(i am sorry, the line numbers in PushDecryptJob are a little bit off in this stacktrace, but i guess you understand my problem)

My master device is signal-cli which i built from master ~3hours ago.

If you want to replicate the problem, just clone my multidevice branch, remove the break and uncomment //group = deviceGroupsInputStream.read(); in PushDecryptJob.java, fire up a vm and load the apk.

Usage is simple, when signal prompts you to enter your phone number click the cancel button, wait a second and scan the qrcode with signal-android or fetch the tsdevice link via logcat, link it to a signal-cli master device, and call signal-cli receive so that it responds the group sync request.

As i said, i got no clue where the problem is, and unfortunately i have neither an android master device at hand (which could hint signal-cli issues, if an android master would work) nor a slave id free for a signal-cli slave (which could tell me if signal-cli slaves can cope with more than 1 synced group). If you could give me a hint on what is going wrong it would be great!

AsamK commented 8 years ago

The issue is caused by the group avatar in the message. The avatar image is directly contained in the groups sync message, not as a separate link. However the GroupMessageProcessor.handleGroupCreate method only handles SignalServiceAttachmentPointer avatars, and not SignalServiceAttachmentStream. Now the avatar is not read from the stream, and the protobuf parser tries to parse the image data, which fails. So the issue is in Signal-Android, not in libsignal-service.

Trolldemorted commented 8 years ago

Thanks a lot, you were completely right! I even checked whether the sent/received group messages were correct and equal (they were, obviously), because i assumed that protobuf would provide me with ready-to-use java objects and would not rely on depleting child stream elements.

Signal-Android does the same, i guess? I went door to door at work, convinced my boss to install Signal-Android and scan my qrcode, and my Signal-Android in an emulator crashed - unfortunately i did not have an adb shell at hand. If that is the case, we might get a seperate fix for that merged, since not being able to handle messages you send yourself is rather odd.

Side note: while browsing through signal-cli i noticed something not worth a seperate issue: this check is redundant, since retrieveGroupAvatarAttachment checks again.

Keep up the good work!