Closed Bug 1230759 Opened 9 years ago Closed 7 years ago

Update libsrtp to version 2.2.0-pre

Categories

(Core :: WebRTC: Networking, defect, P2)

Product:
Core
Component:
WebRTC: Networking
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed
Blocking Flags:
backlog webrtc/webaudio+

People

(Reporter: jesup, Assigned: drno)

References

Details

(Whiteboard: [third-party-lib-audit])

Attachments

(4 files, 10 obsolete files)

Bug 1230759: Part 1 - updated the libsrtp update script
59 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1230759: Part 2 - update libsrtp to 2.2.0-pre
59 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1230759: Part 3 - changes to signaling to work with libsrtp 2.2
59 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1230759: Part 4 - update moz build for libsrtp 2.2
59 bytes, text/x-review-board-request
jesup
: review+
Details 
      No description provided.
backlog: --- → webrtc/webaudio+
Rank: 22
Priority: -- → P2
See Also: → 1245194
Attached patch part-1-fix-update-script.patch (obsolete) — — Splinter Review 
Part 1: libsrtp has moved from SourceForge to GitHub, so fix the update script.

libsrtp 1.5.4 and 2.0.0 were released on 2016-02-02. We currently use libsrtp 1.4.4 from 2012. Changes since version 1.5.0:

https://github.com/cisco/libsrtp/blob/master/CHANGES
Attachment #8732494 - Flags: review?(rjesup)
Part 2: Update libsrtp to version 2.0.0

To build, I needed to:

* remove some deleted .c files from moz.build.
* move some .c files from UNIFIED_SOURCES to SOURCES to avoid redefinition compilation errors.
* ALLOW_COMPILER_WARNINGS because libsrtp 2.0.0 has some new -Wsign-compare warnings.
* define PACKAGE_STRING/VERSION macros now needed to build libsrtp.
* fix SrtpFlow.cpp/h to use the new srtp_* identifier name prefixes.
* export srtp.h for SrtpFlow.cpp/h to use the new type definitions.
Attachment #8732495 - Flags: review?(rjesup)
Blocks: 781787, 1081434
status-firefox46: --- → affected
status-firefox47: --- → affected
status-firefox48: --- → affected
Summary: Update libsrtp to either 1.5.x or trunk from upstream → Update libsrtp to version 2.0.0
FYI, I'm on PTO this week but will get to the review (I've already started)
Attachment #8732494 - Flags: review?(rjesup) → review+
Comment on attachment 8732495 [details] [diff] [review]
part-2-update-srtp-to-2.0.0.patch

Review of attachment 8732495 [details] [diff] [review]:
-----------------------------------------------------------------

r+, but not for checkin at this time.  Before switching branches to the new main 2.x branch, we need to consult with a few people about the stability and state of the 2.0 branch compared to 1.4/1.5 (we could update to the very-stable 1.5.x branch, or do so as an interim step).  
(Note: this records what Chris and I discussed on IRC some time ago)

Also, before landing this, I'd want to go over the patches landed on top of the last import, and see which (if any) of them still should be applied.  (90% of the patches are build-system patches that would be irrelevant).
You can see the list of patches via (on inbound): hg log -r 50ec63e18d1a: netwerk/src

for example, bug 1255655 still applies, so landing this as-is would lose that fix.  I'd also want to consider moving to an update script that (like libvpx) applies our patches on top of an update.
Attachment #8732495 - Flags: review?(rjesup)
ekr - any comments on the timing for updating to the 2.x branch of libsrtp, especially compared to the 1.5.x branch?  We may want to wait for Issue 150 to be resolved upstream, and we may want the buffer-overflow-in-logging fix landed since 2.0.0.

Does 1.5.x have what's needed for PERC?  (Probably not.)

I'll ping Lennox and John Foley as well.
Flags: needinfo?(ekr)
Flags: needinfo?(adam)
(In reply to Randell Jesup [:jesup] from comment #5)
> ekr - any comments on the timing for updating to the 2.x branch of libsrtp,
> especially compared to the 1.5.x branch?  We may want to wait for Issue 150
> to be resolved upstream, and we may want the buffer-overflow-in-logging fix
> landed since 2.0.0.

Sorry, I don't have an informed opinion on this. I could develop one if you need me to...


> Does 1.5.x have what's needed for PERC?  (Probably not.)

No, the transforms will be different, but they're not defined yet.

> I'll ping Lennox and John Foley as well.
Flags: needinfo?(ekr)
What EKR said.
Flags: needinfo?(adam)
Cisco plans to release a beta of libsrtp 2.1 in February or March:

https://github.com/cisco/libsrtp/issues/225

https://github.com/cisco/libsrtp/wiki/Release-2.1
To be able to either use existing libsrtp code or only make minimal changes to support PERC we would at least need release 2.0.0 of libsrtp, because I think that is the first release which brings support for GCM ciphers.
But as release 2.1.0 makes backward incompatible changes to GCM, I think we should skip 2.0.0 and go for for 2.1.0 (or later).
libsrtp 1.6.0 was released today:

"This release restores compatibility between version 1 and version 2.1.0 and later. From this point 1.5.x is effectively EOL and there will be no more changes to 1_5_x_throttle. It is strongly recommended for people currently using a 1.5.x release to move to the 1.6.0 release if they are unable to move the 2.1.0 release."

https://github.com/cisco/libsrtp/issues/280

1.6.0 changelog:
https://github.com/cisco/libsrtp/pull/325/files/ed63d3e9e509f379b84118819b0fde6cae6e3aa7

tagged release:
https://github.com/cisco/libsrtp/releases/tag/v1.6.0
Whiteboard: [third-party-lib-audit]
Mass change P2->P3 to align with new Mozilla triage process.
Priority: P2 → P3
I'm working on a new patch set to update to the latest libsrtp so we can benefit from their latest memory management related fixes.
Assignee: rjesup → drno
Priority: P3 → P2
Attachment #8927615 - Flags: review?(rjesup)
Attachment #8927616 - Flags: review?(rjesup)
Attachment #8927617 - Flags: review?(rjesup)
Attachment #8927618 - Flags: review?(rjesup)
Attachment #8927619 - Flags: review?(rjesup)
Attachment #8927620 - Flags: review?(rjesup)
Attachment #8927621 - Flags: review?(rjesup)
Attachment #8927622 - Flags: review?(rjesup)
Attachment #8927623 - Flags: review?(rjesup)
Attachment #8927624 - Flags: review?(rjesup)
Attachment #8927625 - Flags: review?(rjesup)
Attachment #8927626 - Flags: review?(rjesup)
Blocks: 1416534
Blocks: 1416536
Summary: Update libsrtp to version 2.0.0 → Update libsrtp to version 2.2.0-pre
Comment on attachment 8927623 [details]
Bug 1230759: Part 9 - reapplied patches from bug 792325

https://reviewboard.mozilla.org/r/198926/#review203928

According to libsrtp unit tests this patch actually breaks the roll over counter.
See https://github.com/cisco/libsrtp/pull/382
Attachment #8927623 - Attachment is obsolete: true
Attachment #8927623 - Flags: review?(rjesup)
Comment on attachment 8927618 [details]
Bug 1230759: Part 4 - added missing ifdef's

https://reviewboard.mozilla.org/r/198916/#review204340
Attachment #8927618 - Flags: review?(rjesup) → review+
Comment on attachment 8927615 [details]
Bug 1230759: Part 1 - updated the libsrtp update script

https://reviewboard.mozilla.org/r/198910/#review204346

::: netwerk/srtp/update_srtp.sh:16
(Diff revision 1)
>  if [ "$1" ] ; then
>    export date=`date`
> -#  export revision=`(cd $1; svnversion -n)`
> -  export CVSREAD=0
>    cp -rf $1/srtp $1/crypto $1/include $1/VERSION $1/LICENSE $1/README $1/configure.in netwerk/srtp/src
>  
>    hg addremove netwerk/srtp/src --include "netwerk/srtp/src/VERSION" --include "netwerk/srtp/src/LICENSE" --include "netwerk/srtp/src/configure.in" --include "netwerk/srtp/src/README" --include "**.c" --include "**.h" --similarity 90
>  
> -  echo "srtp updated from CVS on $date" >> netwerk/srtp/srtp_update.log
> -  echo "srtp updated from CVS on $date"
> +  echo "srtp updated from git on $date" >> netwerk/srtp/srtp_update.log
> +  echo "srtp updated from git on $date"
>    echo "WARNING: reapply any local patches!"
>  else
>    echo "usage: $0 path_to_srtp_directory"
>    echo "run from the root of your m-c clone"
>  fi

perhaps add:
  export revision=`(cd $1; git log --pretty=oneline | head -1 | cut -c 1-40)`

and

  echo "srtp updated to version $revision from git on $date" >> netwerk/srtp/srtp_update.log
  echo "srtp updated to version $revision from git on $date"

(I have those changes queued for the sctp update script which is close-to-identical)
Attachment #8927615 - Flags: review?(rjesup) → review+
Comment on attachment 8927616 [details]
Bug 1230759: Part 2 - update libsrtp to 2.2.0-pre

https://reviewboard.mozilla.org/r/198912/#review204350

Maybe update the log message to include the git revision (and maybe date pulled, though revision is more important)?
Attachment #8927616 - Flags: review?(rjesup) → review+
Comment on attachment 8927617 [details]
Bug 1230759: Part 3 - fixed compiler errors in libsrtp

https://reviewboard.mozilla.org/r/198914/#review204444
Attachment #8927617 - Flags: review?(rjesup) → review+
Comment on attachment 8927620 [details]
Bug 1230759: Part 3 - changes to signaling to work with libsrtp 2.2

https://reviewboard.mozilla.org/r/198920/#review205020
Attachment #8927620 - Flags: review?(rjesup) → review+
Comment on attachment 8927621 [details]
Bug 1230759: Part 4 - update moz build for libsrtp 2.2

https://reviewboard.mozilla.org/r/198922/#review205026

::: netwerk/srtp/src/moz.build:43
(Diff revision 2)
> +# This is needed to enable the be32_to_cpu and be64_to_cpu
> +# macro's in datatypes.h
> +DEFINES['HAVE_CONFIG_H'] = 1

Amusing since I added the original NO_64BIT_MATH code for the TI DSP (40-bit LONG LONGs)
Attachment #8927621 - Flags: review?(rjesup) → review+
Comment on attachment 8927619 [details]
Bug 1230759: Part 5 - added comments to ifdef's

https://reviewboard.mozilla.org/r/198918/#review205850
Attachment #8927619 - Flags: review?(rjesup)
Comment on attachment 8927622 [details]
Bug 1230759: Part 8 - fix compiler warnings in libsrtp

https://reviewboard.mozilla.org/r/198924/#review205852
Attachment #8927622 - Flags: review?(rjesup)
Comment on attachment 8927624 [details]
Bug 1230759: Part 9 - reapplied patches for bug 789858

https://reviewboard.mozilla.org/r/198928/#review205856
Attachment #8927624 - Flags: review?(rjesup)
Comment on attachment 8927625 [details]
Bug 1230759: Part 10 - reapplied patches from bug 1003929

https://reviewboard.mozilla.org/r/198930/#review205858
Attachment #8927625 - Flags: review?(rjesup)
Comment on attachment 8927626 [details]
Bug 1230759: Part 11 - reapplied patches from bug 1255655

https://reviewboard.mozilla.org/r/198932/#review205860
Attachment #8927626 - Flags: review?(rjesup)
Attachment #8927618 - Attachment is obsolete: true
Attachment #8927619 - Attachment is obsolete: true
Attachment #8927622 - Attachment is obsolete: true
Attachment #8927624 - Attachment is obsolete: true
Attachment #8927625 - Attachment is obsolete: true
Attachment #8927626 - Attachment is obsolete: true
Attachment #8732495 - Attachment is obsolete: true
Attachment #8732494 - Attachment is obsolete: true
Forgot to submit part 3 when upstreaming our fixes. I wait for https://github.com/cisco/libsrtp/pull/386 to get accepted and merged so we will have a clean import.
Attachment #8927617 - Attachment is obsolete: true
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/3ad217d737b3
Part 1 - updated the libsrtp update script r=jesup
https://hg.mozilla.org/integration/autoland/rev/8a2d5de90bb0
Part 2 - update libsrtp to 2.2.0-pre r=jesup
https://hg.mozilla.org/integration/autoland/rev/807fe0f2d39c
Part 3 - changes to signaling to work with libsrtp 2.2 r=jesup
https://hg.mozilla.org/integration/autoland/rev/f6496640e306
Part 4 - update moz build for libsrtp 2.2 r=jesup
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: