Opened 3 months ago

Closed 4 weeks ago

Last modified 3 weeks ago

#33576 closed defect (fixed)

Update pion-webrtc version to 2.2.3

Reported by: cohosh Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: snowflake, TorBrowserTeam202004R, tbb-9.5a12
Cc: dcf, phw, cohosh Actual Points:
Parent ID: Points:
Reviewer: boklm Sponsor:

Description

We recently tracked down some issues with the pion library that were causing inefficiencies and infinite loops to occur (see #33211).

These have been addressed in pion-dtls v2.0.0-rc.7 and pion-sctp v1.7.5. I'd suggest just bumping to the latest version of pion-webrtc v2.2.3, which includes version bumps for each of these supporting libraries.

Child Tickets

Attachments (1)

gomodtorbm (5.2 KB) - added by cohosh 2 months ago.

Download all attachments as: .zip

Change History (20)

Changed 2 months ago by cohosh

Attachment: gomodtorbm added

comment:1 Changed 2 months ago by cohosh

Here's a patch that updates it using the script method from #28942: https://gitweb.torproject.org/user/cohosh/tor-browser-build.git/commit/?h=bug/33576&id=282f699cd4079a8f6947876e1005b318a3349fb8

I had to make a few modifications to the script to account for new patters in the output of go mod graph, I've attached the script to this ticket. I'm going to take a look and see if we can prune some of the dependencies. dcf pointed out in #33330 that some of the dependencies are only needed for tests.

comment:2 Changed 2 months ago by cohosh

I was able to remove 3 dependencies for tor browser (these were indirect depends from gomega). I don't know of a way of automating it with any of the go module tools, but at least this cuts down a bit on size for now.

Here's the branch (with two commits) to review: https://gitweb.torproject.org/user/cohosh/tor-browser-build.git/log/?h=bug/33330

comment:3 Changed 2 months ago by cohosh

Owner: changed from cohosh to tbb-team

comment:4 Changed 2 months ago by cohosh

Status: assignedneeds_review

comment:5 Changed 2 months ago by gk

Keywords: TorBrowserTeam202003R added

comment:6 Changed 8 weeks ago by pili

Keywords: TorBrowserTeam202004R added; TorBrowserTeam202003R removed

We are no longer in March

comment:7 Changed 7 weeks ago by pili

Reviewer: boklm

comment:8 Changed 4 weeks ago by cohosh

Status: needs_reviewneeds_revision

Rebasing because of #33761

comment:9 Changed 4 weeks ago by cohosh

Status: needs_revisionneeds_review

I rebased the branch. It's actually just one commit now: https://gitweb.torproject.org/user/cohosh/tor-browser-build.git/commit/?h=bug/33330&id=8d7c51141c71923e0b62043a62f977c0254e679a

I started a build to test it, I'll update here when it's done

comment:10 Changed 4 weeks ago by sysrqb

Status: needs_reviewneeds_information

This all looks good except for two pieces.

diff --git a/projects/pion-webrtc/config b/projects/pion-webrtc/config
index c27a3df1..d66b3bf1 100644
--- a/projects/pion-webrtc/config
+++ b/projects/pion-webrtc/config
@@ -1,7 +1,7 @@
 # vim: filetype=yaml sw=2
 version: '[% c("abbrev") %]'
 git_url: https://github.com/pion/webrtc
-git_hash: 77c6e3b827e40175572fcf2894bb81ab1e8fd2c9 # v2.1.18
+git_hash: 1f448413f22d6c32d690db594748d8ad5d3812f5 # v2.1.2

The hash 1f448413f22d6c32d690db594748d8ad5d3812f5 is tagged as v2.2.3: https://github.com/pion/webrtc/commit/1f448413f22d6c32d690db594748d8ad5d3812f5

And bumping webrtc to v2.2.3 is the purpose of this ticket, so that's all...fine :) However, I wonder why go mod graph (or the script) think v2.1.2 is the/a new dependency version. Do you know?

Second, do you know where the crypto commit hash bump came from? Was that a manual change you did? I found that hash on the master branch, so I don't have a problem with using it (I see the current commit we're using is pretty old).

diff --git a/projects/goxcrypto/config b/projects/goxcrypto/config
index 698f94b1..83e3e7fe 100644
--- a/projects/goxcrypto/config
+++ b/projects/goxcrypto/config
@@ -1,7 +1,7 @@
 # vim: filetype=yaml sw=2
 version: '[% c("abbrev") %]'
 git_url: https://go.googlesource.com/crypto
-git_hash: ff983b9c42bc9fbf91556e191cc8efb585c16908
+git_hash: e7c4368fe9ddd156b5f1463283cb51c5b400c373
 filename: '[% project %]-[% c("version") %]-[% c("var/osname") %]-[% c("var/build_id") %].tar.gz'
 
 build: '[% c("projects/go/var/build_go_lib") %]'

comment:11 in reply to:  10 Changed 4 weeks ago by cohosh

Replying to sysrqb:

And bumping webrtc to v2.2.3 is the purpose of this ticket, so that's all...fine :) However, I wonder why go mod graph (or the script) think v2.1.2 is the/a new dependency version. Do you know?

That's just a typo on part I think. The commit has matches v2.2.3. Sorry about that. It wasn't generated by gomodtorbm since it's the root library we're updating.

Second, do you know where the crypto commit hash bump came from? Was that a manual change you did? I found that hash on the master branch, so I don't have a problem with using it (I see the current commit we're using is pretty old).

Hmm. Looks like it was bumped here.
But this isn't even the most recent version bump for v2.2.3. And now that I'm looking at it, gomodtorbm should've ignored that project.

Perhaps we shouldn't update it to be on the safe side.

I'll make these changes.

comment:12 Changed 4 weeks ago by cohosh

Alright a few notes on the changes:

  • my rebase mangled things a bit and I lost some new packages that are necessary to build v2.2.3. Those have been re-added.
  • The bump in goxcrypto was done manually by me for a reason. Normally gomodtorbm ignores existing projects that are shared by other parts of tor browser (including goxcrypto). However, in this case, the move to pion-dtls v2.0.0-rc.7 requires a newer version of goxcrypto to build (notably, a commit in which curve25519.X25519 is defined). So I've bumped the version to the most recent needed by pion-dtls.
  • The reason gomodtorbm thought the pion-webrtc version should be v2.1.2 is because it was set in the script from the last time we updated it. I manually edited the script to be v.2.2.3 and it works as expected now :)

I've updated my patch: https://gitweb.torproject.org/user/cohosh/tor-browser-build.git/commit/?h=bug/33330&id=90bf7e4dd6779e563c71557934e571a48889c8da

I've made sure pion-webrtc and obfs4 build with this update, which should be the only projects that use goxcrypto.

comment:13 Changed 4 weeks ago by sysrqb

Keywords: tbb-9.5a12 added
Resolution: fixed
Status: needs_informationclosed

Thanks! It looks good now.

Some notes for myself:

crypto: 69ecbb4d6d5dab05e49161c6e77ea40a030884e1 - https://go.googlesource.com/crypto/+/refs/heads/release-branch.go1.14

xerrors: commit is HEAD on master: https://github.com/golang/xerrors/commit/9bdfabe68543c54f90421aeb9a60ef8061b5b544

And I tweaked the branch a little. The comment documenting the tag on the git_hash for webrtc was still using the old, hardcoded, number. I manually changed that.

diff --git a/projects/pion-webrtc/config b/projects/pion-webrtc/config
index d66b3bf1..f4bde825 100644
--- a/projects/pion-webrtc/config
+++ b/projects/pion-webrtc/config
@@ -1,7 +1,7 @@
 # vim: filetype=yaml sw=2
 version: '[% c("abbrev") %]'
 git_url: https://github.com/pion/webrtc
-git_hash: 1f448413f22d6c32d690db594748d8ad5d3812f5 # v2.1.2
+git_hash: 1f448413f22d6c32d690db594748d8ad5d3812f5 # v2.2.3
 filename: '[% project %]-[% c("version") %]-[% c("var/osname") %]-[% c("var/build_id") %].tar.gz'
 
 build: '[% c("projects/go/var/build_go_lib") %]'

This is now 02a73e328b08882cff7a20cedac446aad423de35 on master.

comment:14 Changed 4 weeks ago by sysrqb

Resolution: fixed
Status: closedreopened

Hrm.

http://f4amtbsowhix7rrf.onion/reports/r/tor-browser-2020-04-29/tor-browser_build.html

Cloning into 'goxxerrors'...
fatal: https://golang.org/x/xerrors/info/refs not valid: is this a git repository?
Error: Error cloning https://golang.org/x/xerrors
make: *** [fetch] Error 1

I'm guessing this is because https://golang.org/x/xerrors redirects to https://godoc.org/golang.org/x/xerrors, but the first page contains metadata for the actual git repo. I can only assume the go tooling understands this magic:

<meta name="go-import" content="golang.org/x/xerrors git https://go.googlesource.com/xerrors">
<meta name="go-source" content="golang.org/x/xerrors https://github.com/golang/xerrors/ https://github.com/golang/xerrors/tree/master{/dir} https://github.com/golang/xerrors/blob/master{/dir}/{file}#L{line}">

We can try using https://go.googlesource.com/xerrors or https://github.com/golang/xerrors/ instead.

comment:15 Changed 4 weeks ago by cohosh

That's weird. The build worked for me o.O I wonder why

comment:16 Changed 4 weeks ago by cohosh

Status: reopenedneeds_review

Just did a nightly build of the snowflake project for linux and it worked:

https://gitweb.torproject.org/user/cohosh/tor-browser-build.git/commit/?h=ticket/33576&id=be5efccdf6aeb316a2b8c9888a942e647c544bc9

I figured out what likely went wrong: I probably had the right git_url the first time I built it as an alpha testbuild. And then re-ran gomodtorbm which gave me the incorrect url, but the alpha testbuild didn't notice because the git hash didn't change. That's my current theory and I've switched to nightly testbuilds so this doesn't happen again, hopefully :)

comment:17 Changed 4 weeks ago by sysrqb

Resolution: fixed
Status: needs_reviewclosed

Looks good! Thanks! I merged it with commit 7464798ed3a34a2d492c55e34673dbe3d4a134d2.

comment:18 Changed 3 weeks ago by cypherpunks

Bug 33573: Update pion-webrtc version to 2.2.3 in changelog.

comment:19 in reply to:  18 Changed 3 weeks ago by gk

Replying to cypherpunks:

Bug 33573: Update pion-webrtc version to 2.2.3 in changelog.

Thanks. Fixed both in tor-browser-build and the blog post.

Note: See TracTickets for help on using tickets.