779018 - Need to be able to send and receive DTLS fingerprint in SDP

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[enda mannion (:emannion)]

Attachment: DTLS added to SIPCC SDP

gsmsdp_install_peer_dtls_data_attributes  is used to set fingerprint data into VCM layer.

vcmGetDtlsIdentity is used to get fingeprint data.

There is still wor to do around these functions.

Not sure if I am getting and setting data in all the right places. Let me know.

Comment 1

[Ethan Hugg [:ehugg]]

Comment on attachment 647395 [details] [diff] [review]
DTLS added to SIPCC SDP

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

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +900,5 @@
> +                size_t *max_digest_alg_len,
> +                char **digestp,
> +                size_t *max_digest_len) {
> +
> +  CSFLogDebug( logTag, "vcmGetDtlsIdentity: PC = %s", peerconnection);

We should use __FUNCTION__ here.

@@ +903,5 @@
> +
> +  CSFLogDebug( logTag, "vcmGetDtlsIdentity: PC = %s", peerconnection);
> +
> +  *digest_algp = *digestp = NULL;
> +  *max_digest_alg_len = 10;   // <emannion> sorry for the crap sizing

I suppose you could size these to match what you're putting into it if it's going to stay coming from a literal like "SHA-1"

@@ +914,5 @@
> +
> +  digest_alg = (char *)malloc(*max_digest_alg_len);
> +  if (!digest_alg)
> +    return VCM_ERROR;
> +  strncpy(digest_alg, "SHA-1", *max_digest_alg_len);

We should be using sstrncpy here (and anywhere there's a strcpy or strncpy).  It guarantees a null at the end.  In fact you use it later in this patch.

@@ +920,5 @@
> +
> +  digest = (char *)malloc(*max_digest_len);
> +  if (!digest)
> +    return VCM_ERROR;
> +  strncpy(digest, "4A:AD:B9:B1:3F:82:18:3B:54:02:12:DF:3E:5D:49:6B:19:E5:7C:AB", *max_digest_len);

sstrncpy here as well

@@ +929,5 @@
> +    if (digest_alg)
> +      free(digest_alg);
> +    if (digest)
> +      free(digest);
> +    CSFLogDebug( logTag, "vcmGetDtlsIdentity(): no digest_alg/digest");

__FUNCTION__

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +2930,5 @@
> +        return SM_RC_END;
> +
> +    dcb->max_digest_alg_len = max_digest_alg_len;
> +
> +    strncpy(dcb->digest_alg, digest_alg, max_digest_alg_len + 1);

This strncpy could write past the amount you malloc'd.  In general you should use sstrncpy with the third param being exactly the size of the buffer you're copying into.

Also, is there a reason why you wouldn't want to just pass in &dcb->digest_alg and &dcb->digest into vcmGetDtlsIdentity and have them malloc'd once there?  You could save a couple mallocs and params, or are you planning for the future?

@@ +2939,5 @@
> +        return SM_RC_END;
> +
> +    dcb->max_digest_len = max_digest_len;
> +
> +    strncpy(dcb->digest, digest, max_digest_len + 1);

Same here, either lose the +1 or add it also to the malloc.

@@ +3051,5 @@
> +        return SM_RC_END;
> +
> +    dcb->max_digest_alg_len = max_digest_alg_len;
> +
> +    strncpy(dcb->digest_alg, digest_alg, max_digest_alg_len + 1);

Same here

@@ +3060,5 @@
> +        return SM_RC_END;
> +
> +    dcb->max_digest_len = max_digest_len;
> +
> +    strncpy(dcb->digest, digest, max_digest_len + 1);

and here

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +5734,5 @@
> +    line_to_split = (char*)cpr_malloc(len);
> +    if(!line_to_split)
> +    	return CC_CAUSE_ERROR;
> +
> +    strncpy(line_to_split, fingerprint, len + 1);

and here

@@ +5741,5 @@
> +    parta = (char*)cpr_malloc(sizeof(char*) * dcb_p->max_digest_alg_len);
> +    if(!parta)
> +    	return CC_CAUSE_ERROR;
> +
> +    strncpy(parta, token, dcb_p->max_digest_alg_len + 1);

here

@@ +5746,5 @@
> +
> +    token = strtok(NULL, delim);
> +    partb = (char*)cpr_malloc(sizeof(char*) * dcb_p->max_digest_len);
> +    if(!partb)
> +        return CC_CAUSE_ERROR;

Here you are returning without freeing line_to_split or parta.  Perhaps that's a rare case, but leaking memory when out-of-memory won't help.

@@ +5748,5 @@
> +    partb = (char*)cpr_malloc(sizeof(char*) * dcb_p->max_digest_len);
> +    if(!partb)
> +        return CC_CAUSE_ERROR;
> +
> +    strncpy(partb, token, dcb_p->max_digest_len + 1);

here

Comment 2

[enda mannion (:emannion)]

(In reply to Ethan Hugg [:ehugg] from comment #1)
> Comment on attachment 647395 [details] [diff] [review]
> DTLS added to SIPCC SDP
> 
> Review of attachment 647395 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
> @@ +900,5 @@
> > +                size_t *max_digest_alg_len,
> > +                char **digestp,
> > +                size_t *max_digest_len) {
> > +
> > +  CSFLogDebug( logTag, "vcmGetDtlsIdentity: PC = %s", peerconnection);
> 
> We should use __FUNCTION__ here.

Done

> 
> @@ +903,5 @@
> > +
> > +  CSFLogDebug( logTag, "vcmGetDtlsIdentity: PC = %s", peerconnection);
> > +
> > +  *digest_algp = *digestp = NULL;
> > +  *max_digest_alg_len = 10;   // <emannion> sorry for the crap sizing
> 
> I suppose you could size these to match what you're putting into it if it's
> going to stay coming from a literal like "SHA-1"
> 

ekr: will fill out the rest this function.

> @@ +914,5 @@
> > +
> > +  digest_alg = (char *)malloc(*max_digest_alg_len);
> > +  if (!digest_alg)
> > +    return VCM_ERROR;
> > +  strncpy(digest_alg, "SHA-1", *max_digest_alg_len);
> 
> We should be using sstrncpy here (and anywhere there's a strcpy or strncpy).
> It guarantees a null at the end.  In fact you use it later in this patch.
> 

this file uses strncopy everywhere else.

> @@ +920,5 @@
> > +
> > +  digest = (char *)malloc(*max_digest_len);
> > +  if (!digest)
> > +    return VCM_ERROR;
> > +  strncpy(digest, "4A:AD:B9:B1:3F:82:18:3B:54:02:12:DF:3E:5D:49:6B:19:E5:7C:AB", *max_digest_len);
> 
> sstrncpy here as well
> 
> @@ +929,5 @@
> > +    if (digest_alg)
> > +      free(digest_alg);
> > +    if (digest)
> > +      free(digest);
> > +    CSFLogDebug( logTag, "vcmGetDtlsIdentity(): no digest_alg/digest");
> 
> __FUNCTION__
> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
> @@ +2930,5 @@
> > +        return SM_RC_END;
> > +
> > +    dcb->max_digest_alg_len = max_digest_alg_len;
> > +
> > +    strncpy(dcb->digest_alg, digest_alg, max_digest_alg_len + 1);
> 
> This strncpy could write past the amount you malloc'd.  In general you
> should use sstrncpy with the third param being exactly the size of the
> buffer you're copying into.
> 

Done.

> Also, is there a reason why you wouldn't want to just pass in
> &dcb->digest_alg and &dcb->digest into vcmGetDtlsIdentity and have them
> malloc'd once there?  You could save a couple mallocs and params, or are you
> planning for the future?
> 

I tried but was getting problems where the dcb was getting released. So maybe in time I can work on that again. Plus I want ekr to fill out vcmGetDtlsIdentity.

> @@ +2939,5 @@
> > +        return SM_RC_END;
> > +
> > +    dcb->max_digest_len = max_digest_len;
> > +
> > +    strncpy(dcb->digest, digest, max_digest_len + 1);
> 
> Same here, either lose the +1 or add it also to the malloc.

Done.

> 
> @@ +3051,5 @@
> > +        return SM_RC_END;
> > +
> > +    dcb->max_digest_alg_len = max_digest_alg_len;
> > +
> > +    strncpy(dcb->digest_alg, digest_alg, max_digest_alg_len + 1);
> 
> Same here

Done.

> 
> @@ +3060,5 @@
> > +        return SM_RC_END;
> > +
> > +    dcb->max_digest_len = max_digest_len;
> > +
> > +    strncpy(dcb->digest, digest, max_digest_len + 1);
> 
> and here
> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
> @@ +5734,5 @@
> > +    line_to_split = (char*)cpr_malloc(len);
> > +    if(!line_to_split)
> > +    	return CC_CAUSE_ERROR;
> > +
> > +    strncpy(line_to_split, fingerprint, len + 1);
> 
> and here
> 
> @@ +5741,5 @@
> > +    parta = (char*)cpr_malloc(sizeof(char*) * dcb_p->max_digest_alg_len);
> > +    if(!parta)
> > +    	return CC_CAUSE_ERROR;
> > +
> > +    strncpy(parta, token, dcb_p->max_digest_alg_len + 1);
> 
> here

done.

> 
> @@ +5746,5 @@
> > +
> > +    token = strtok(NULL, delim);
> > +    partb = (char*)cpr_malloc(sizeof(char*) * dcb_p->max_digest_len);
> > +    if(!partb)
> > +        return CC_CAUSE_ERROR;
> 
> Here you are returning without freeing line_to_split or parta.  Perhaps
> that's a rare case, but leaking memory when out-of-memory won't help.
> 

got it now.

> @@ +5748,5 @@
> > +    partb = (char*)cpr_malloc(sizeof(char*) * dcb_p->max_digest_len);
> > +    if(!partb)
> > +        return CC_CAUSE_ERROR;
> > +
> > +    strncpy(partb, token, dcb_p->max_digest_len + 1);
> 
> here

Comment 3

[enda mannion (:emannion)]

Attachment: Revised: DTLS added to SIPCC SDP

I added some warning fixes to this patch.

Comment 4

[Ethan Hugg [:ehugg]]

>> @@ +914,5 @@
>> > +
>> > +  digest_alg = (char *)malloc(*max_digest_alg_len);
>> > +  if (!digest_alg)
>> > +    return VCM_ERROR;
>> > +  strncpy(digest_alg, "SHA-1", *max_digest_alg_len);
>> 
>> We should be using sstrncpy here (and anywhere there's a strcpy or strncpy).
>> It guarantees a null at the end.  In fact you use it later in this patch.
>> 
>
>this file uses strncopy everywhere else.

If you look at the diff of your patch you'll see you are adding code with sstrncpy in this patch.  This is a good thing because you are guaranteeing a null-terminated string.  Regular strncpy does not guarantee a null-term string.  This was the point of my earlier patch that is not yet applied where I got rid of all strcpy and strncpy in favor of sstrncpy throughout the signaling code.

Comment 5

[Eric Rescorla (:ekr)]

Comment on attachment 647535 [details] [diff] [review]
Revised: DTLS added to SIPCC SDP

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

Also, please update the unit tests to make sure this works. I.e. check for fingerprint.

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +2920,5 @@
>      strncpy(dcb->ice_pwd, ice_pwd, strlen(ice_pwd) + 1);
>      free(ice_pwd);
>  
> +
> +    vcmGetDtlsIdentity(dcb->peerconnection,

Error check needed here.

@@ +2931,5 @@
> +
> +    dcb->max_digest_alg_len = max_digest_alg_len;
> +
> +    strncpy(dcb->digest_alg, digest_alg, max_digest_alg_len + 1);
> +    free(digest_alg);

Probably better to:

1. Use sstrncpy here.
2. Have an API where we pass down the buffers with max lengths and they get filled in. This avoids spurious mallocs and disagreements about length.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +1496,5 @@
> +		int hash_func_len, char *fingerprint, int fingerprint_len)
> +{
> +    uint16_t      a_instance = 0;
> +    sdp_result_e  result;
> +    char         *hash_and_fingerprint = cpr_malloc(sizeof(char) * ((hash_func_len + fingerprint_len) + 2));

sizeof(char) == 1 by definition so you can omit this sizeof()

For that matter, since hash_func_len < 100, why not just put this in the stack? That way you avoid the memory leak you are creating here by not freeing hash_and_fingerprint.

@@ +5729,5 @@
> +
> +    if (sdp_res != SDP_SUCCESS)
> +        return CC_CAUSE_ERROR;
> +
> +    len = dcb_p->max_digest_alg_len + dcb_p->max_digest_len + 2;

Again, why not just allocate these buffers on the stack?

@@ +5772,5 @@
> +        continue;
> +
> +      sdp_res = sdp_attr_get_dtls_fingerprint_attribute (sdp_p->dest_sdp, media->level,
> +                                        0, SDP_ATTR_DTLS_FINGERPRINT, 1, &fingerprint);
> +    }

This code doesn't look right. You need to conditionally install either the session-level or media-level attributes. I don't think this does that.

Comment 6

[enda mannion (:emannion)]

(In reply to Eric Rescorla from comment #5)
> Comment on attachment 647535 [details] [diff] [review]
> Revised: DTLS added to SIPCC SDP
> 
> Review of attachment 647535 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Also, please update the unit tests to make sure this works. I.e. check for
> fingerprint.

done

> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
> @@ +2920,5 @@
> >      strncpy(dcb->ice_pwd, ice_pwd, strlen(ice_pwd) + 1);
> >      free(ice_pwd);
> >  
> > +
> > +    vcmGetDtlsIdentity(dcb->peerconnection,
> 
> Error check needed here.

done

> 
> @@ +2931,5 @@
> > +
> > +    dcb->max_digest_alg_len = max_digest_alg_len;
> > +
> > +    strncpy(dcb->digest_alg, digest_alg, max_digest_alg_len + 1);
> > +    free(digest_alg);
> 
> Probably better to:
> 
> 1. Use sstrncpy here.
> 2. Have an API where we pass down the buffers with max lengths and they get
> filled in. This avoids spurious mallocs and disagreements about length.

done

> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
> @@ +1496,5 @@
> > +		int hash_func_len, char *fingerprint, int fingerprint_len)
> > +{
> > +    uint16_t      a_instance = 0;
> > +    sdp_result_e  result;
> > +    char         *hash_and_fingerprint = cpr_malloc(sizeof(char) * ((hash_func_len + fingerprint_len) + 2));
> 
> sizeof(char) == 1 by definition so you can omit this sizeof()
> 
> For that matter, since hash_func_len < 100, why not just put this in the
> stack? That way you avoid the memory leak you are creating here by not
> freeing hash_and_fingerprint.

wont work when I put on the stack, so now I make sure I free the heap memory.


> 
> @@ +5729,5 @@
> > +
> > +    if (sdp_res != SDP_SUCCESS)
> > +        return CC_CAUSE_ERROR;
> > +
> > +    len = dcb_p->max_digest_alg_len + dcb_p->max_digest_len + 2;
> 
> Again, why not just allocate these buffers on the stack?

not working on the stack.

> 
> @@ +5772,5 @@
> > +        continue;
> > +
> > +      sdp_res = sdp_attr_get_dtls_fingerprint_attribute (sdp_p->dest_sdp, media->level,
> > +                                        0, SDP_ATTR_DTLS_FINGERPRINT, 1, &fingerprint);
> > +    }
> 
> This code doesn't look right. You need to conditionally install either the
> session-level or media-level attributes. I don't think this does that.

I left this code for ekr to finish as you know the VCM interface to set the fingerprint into.

Comment 7

[enda mannion (:emannion)]

Attachment: revised: DTLS added to SIPCC SDP

Comment 8

[Ethan Hugg [:ehugg]]

Attachment: send and receive DTLS fingerprint in SDP

Comment 9

[Ethan Hugg [:ehugg]]

Comment on attachment 652216 [details] [diff] [review]
send and receive DTLS fingerprint in SDP


Changed hash_and_fingerprint to go on the stack, vcmGetDtlsIdentity takes char* instead of char**, more use of sstrncpy.

Comment 10

[Ethan Hugg [:ehugg]]

Attachment: send and receive DTLS fingerprint in SDP

Comment 11

[Ethan Hugg [:ehugg]]

Attachment: send and receive DTLS fingerprint in SDP

Comment 12

[Ethan Hugg [:ehugg]]

Attachment: send and receive DTLS fingerprint in SDP

Comment 13

[Ethan Hugg [:ehugg]]

Comment on attachment 652256 [details] [diff] [review]
send and receive DTLS fingerprint in SDP


Moved the length definitions out of dcb and into #defs.  Moved a few more things onto the stack from being allocated.  Changed a few log messages to use __FUNCTION__ so they report their correct names.

Comment 14

[Ethan Hugg [:ehugg]]

Pushed to Alder - https://hg.mozilla.org/projects/alder/rev/755ac3bb6281

Comment 15

[enda mannion (:emannion)]

Attachment: Add DTLS data to VCMStartICE parameters

Comment 16

[Ethan Hugg [:ehugg]]

Attachment: Remove variable array from gsm_sdp for Windows build

Comment 17

[enda mannion (:emannion)]

Attachment: revised: add DTLS fingerprint to VCMStartICE parameters

Comment 18

[Eric Rescorla (:ekr)]

Comment on attachment 652908 [details] [diff] [review]
revised: add DTLS fingerprint to VCMStartICE parameters

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

This still needs some cleanup. Comments below.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +5729,4 @@
>      sdp_res = sdp_attr_get_dtls_fingerprint_attribute (sdp_p->dest_sdp, SDP_SESSION_LEVEL,
>                                        0, SDP_ATTR_DTLS_FINGERPRINT, 1, &fingerprint);
>  
> +    if (SDP_SUCCESS == sdp_res) {

I suggest refactoring this code so that you don't have to do the split twice. How about storing sdp_res instead and then having a common splitter?

Also, this logic is incorrect because the media lines override the session, not vide versa.

@@ +5740,5 @@
> +        GSMSDP_FOR_ALL_MEDIA(media, dcb_p) {
> +          if (!GSMSDP_MEDIA_ENABLED(media))
> +            continue;
> +
> +          media->negotiated_crypto.algorithmID = VCM_SHA_1_ALG;

What happens if this is not SHA-1? You need to look this up in a table.

@@ +5756,4 @@
>                                          0, SDP_ATTR_DTLS_FINGERPRINT, 1, &fingerprint);
> +
> +          if (SDP_SUCCESS == sdp_res) {
> +              sstrncpy(line_to_split, fingerprint, FSMDEF_MAX_DIGEST_ALG_LEN + FSMDEF_MAX_DIGEST_LEN + 2);

Use sizeof() here.

@@ +5756,5 @@
>                                          0, SDP_ATTR_DTLS_FINGERPRINT, 1, &fingerprint);
> +
> +          if (SDP_SUCCESS == sdp_res) {
> +              sstrncpy(line_to_split, fingerprint, FSMDEF_MAX_DIGEST_ALG_LEN + FSMDEF_MAX_DIGEST_LEN + 2);
> +              token = strtok(line_to_split, delim);

What if it's missing?

@@ +5758,5 @@
> +          if (SDP_SUCCESS == sdp_res) {
> +              sstrncpy(line_to_split, fingerprint, FSMDEF_MAX_DIGEST_ALG_LEN + FSMDEF_MAX_DIGEST_LEN + 2);
> +              token = strtok(line_to_split, delim);
> +              sstrncpy(digest_alg, token, FSMDEF_MAX_DIGEST_ALG_LEN);
> +              token = strtok(NULL, delim);

Same here.

::: media/webrtc/signaling/src/sipcc/core/gsm/h/fsm.h
@@ +66,5 @@
>  #define FSMDEF_NO_DCB              (NULL)
>  #define FSMDEF_ERR_ONHOOK_TMR_SECS (20)
>  
> +#define FSMDEF_MAX_DIGEST_ALG_LEN  10
> +#define FSMDEF_MAX_DIGEST_LEN      70

This is slightly too short. The maximum length is actually something like 32 * 3 (32 digits hex encoded + 31 colons + a null terminator) for SHA-512. Maybe 100?

Comment 19

[enda mannion (:emannion)]

(In reply to Eric Rescorla from comment #18)
> Comment on attachment 652908 [details] [diff] [review]
> revised: add DTLS fingerprint to VCMStartICE parameters
> 
> Review of attachment 652908 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This still needs some cleanup. Comments below.
> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
> @@ +5729,4 @@
> >      sdp_res = sdp_attr_get_dtls_fingerprint_attribute (sdp_p->dest_sdp, SDP_SESSION_LEVEL,
> >                                        0, SDP_ATTR_DTLS_FINGERPRINT, 1, &fingerprint);
> >  
> > +    if (SDP_SUCCESS == sdp_res) {
> 
> I suggest refactoring this code so that you don't have to do the split
> twice. How about storing sdp_res instead and then having a common splitter?
> 
> Also, this logic is incorrect because the media lines override the session,
> not vide versa.

Done.

> 
> @@ +5740,5 @@
> > +        GSMSDP_FOR_ALL_MEDIA(media, dcb_p) {
> > +          if (!GSMSDP_MEDIA_ENABLED(media))
> > +            continue;
> > +
> > +          media->negotiated_crypto.algorithmID = VCM_SHA_1_ALG;
> 
> What happens if this is not SHA-1? You need to look this up in a table.

For now I went back to your original API where I pass you the algorithm as a string.

> 
> @@ +5756,4 @@
> >                                          0, SDP_ATTR_DTLS_FINGERPRINT, 1, &fingerprint);
> > +
> > +          if (SDP_SUCCESS == sdp_res) {
> > +              sstrncpy(line_to_split, fingerprint, FSMDEF_MAX_DIGEST_ALG_LEN + FSMDEF_MAX_DIGEST_LEN + 2);
> 
> Use sizeof() here.
> 
> @@ +5756,5 @@
> >                                          0, SDP_ATTR_DTLS_FINGERPRINT, 1, &fingerprint);
> > +
> > +          if (SDP_SUCCESS == sdp_res) {
> > +              sstrncpy(line_to_split, fingerprint, FSMDEF_MAX_DIGEST_ALG_LEN + FSMDEF_MAX_DIGEST_LEN + 2);
> > +              token = strtok(line_to_split, delim);
> 
> What if it's missing?

My time today was more limited than I expected I will add more validation to this soon, there are othe rplaces I need to do this an its noted.

> 
> @@ +5758,5 @@
> > +          if (SDP_SUCCESS == sdp_res) {
> > +              sstrncpy(line_to_split, fingerprint, FSMDEF_MAX_DIGEST_ALG_LEN + FSMDEF_MAX_DIGEST_LEN + 2);
> > +              token = strtok(line_to_split, delim);
> > +              sstrncpy(digest_alg, token, FSMDEF_MAX_DIGEST_ALG_LEN);
> > +              token = strtok(NULL, delim);
> 
> Same here.
> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/h/fsm.h
> @@ +66,5 @@
> >  #define FSMDEF_NO_DCB              (NULL)
> >  #define FSMDEF_ERR_ONHOOK_TMR_SECS (20)
> >  
> > +#define FSMDEF_MAX_DIGEST_ALG_LEN  10
> > +#define FSMDEF_MAX_DIGEST_LEN      70
> 
> This is slightly too short. The maximum length is actually something like 32
> * 3 (32 digits hex encoded + 31 colons + a null terminator) for SHA-512.
> Maybe 100?

Done.

Comment 20

[enda mannion (:emannion)]

Attachment: revised: Add DTLS data to VCMStartICE parameters

Comment 21

[enda mannion (:emannion)]

I had to comment pr_log.h   and PR_ASSERT from gsm_sdp.c as there were some type conflict errors.  I will re-visit this ASAP.

Comment 22

[enda mannion (:emannion)]

Attachment: revised: Add DTLS data to VCMStartICE parameters

Comment 23

[Eric Rescorla (:ekr)]

Comment on attachment 653078 [details] [diff] [review]
revised: Add DTLS data to VCMStartICE parameters

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

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +5708,5 @@
> +        sdp_res = sdp_attr_get_dtls_fingerprint_attribute (sdp_p->dest_sdp, media->level,
> +                                    0, SDP_ATTR_DTLS_FINGERPRINT, 1, &fingerprint);
> +
> +        if (SDP_SUCCESS == sdp_res ) {
> +            sstrncpy(line_to_split, fingerprint, FSMDEF_MAX_DIGEST_ALG_LEN + FSMDEF_MAX_DIGEST_LEN + 2);

Use sizeof() for the buffer lengths here and below.

@@ +5720,5 @@
> +            token = strtok(NULL, delim);
> +            sstrncpy(digest, token, FSMDEF_MAX_DIGEST_LEN);
> +
> +            sstrncpy(media->negotiated_crypto.algorithm, digest_alg, FSMDEF_MAX_DIGEST_ALG_LEN);
> +            sstrncpy(media->negotiated_crypto.digest, digest, FSMDEF_MAX_DIGEST_LEN);

Error checks needed here. IIRC strtok returns NULL if nothing is found.

@@ +5722,5 @@
> +
> +            sstrncpy(media->negotiated_crypto.algorithm, digest_alg, FSMDEF_MAX_DIGEST_ALG_LEN);
> +            sstrncpy(media->negotiated_crypto.digest, digest, FSMDEF_MAX_DIGEST_LEN);
> +        } else {
> +            GSM_DEBUG(DEB_F_PREFIX"DTLS attribute error\n",

We should probably cause this to hard fail at this point, since crypto is required.

@@ +5855,5 @@
>    * of streams, each with its list of tracks and associated data.
>    * Currently this just creates 1 track per 1 stream.
>    */
>  
> +  //PR_ASSERT(idx < CC_MAX_STREAMS);

Why did you change this?

Comment 24

[enda mannion (:emannion)]

(In reply to Eric Rescorla from comment #23)
> Comment on attachment 653078 [details] [diff] [review]
> revised: Add DTLS data to VCMStartICE parameters
> 
> Review of attachment 653078 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
> @@ +5708,5 @@
> > +        sdp_res = sdp_attr_get_dtls_fingerprint_attribute (sdp_p->dest_sdp, media->level,
> > +                                    0, SDP_ATTR_DTLS_FINGERPRINT, 1, &fingerprint);
> > +
> > +        if (SDP_SUCCESS == sdp_res ) {
> > +            sstrncpy(line_to_split, fingerprint, FSMDEF_MAX_DIGEST_ALG_LEN + FSMDEF_MAX_DIGEST_LEN + 2);
> 
> Use sizeof() for the buffer lengths here and below.

These are #defines  sizeof if not working for me here.  I had tried it already.

> 
> @@ +5720,5 @@
> > +            token = strtok(NULL, delim);
> > +            sstrncpy(digest, token, FSMDEF_MAX_DIGEST_LEN);
> > +
> > +            sstrncpy(media->negotiated_crypto.algorithm, digest_alg, FSMDEF_MAX_DIGEST_ALG_LEN);
> > +            sstrncpy(media->negotiated_crypto.digest, digest, FSMDEF_MAX_DIGEST_LEN);
> 
> Error checks needed here. IIRC strtok returns NULL if nothing is found.

Done.

> 
> @@ +5722,5 @@
> > +
> > +            sstrncpy(media->negotiated_crypto.algorithm, digest_alg, FSMDEF_MAX_DIGEST_ALG_LEN);
> > +            sstrncpy(media->negotiated_crypto.digest, digest, FSMDEF_MAX_DIGEST_LEN);
> > +        } else {
> > +            GSM_DEBUG(DEB_F_PREFIX"DTLS attribute error\n",
> 
> We should probably cause this to hard fail at this point, since crypto is
> required.

Done.

> 
> @@ +5855,5 @@
> >    * of streams, each with its list of tracks and associated data.
> >    * Currently this just creates 1 track per 1 stream.
> >    */
> >  
> > +  //PR_ASSERT(idx < CC_MAX_STREAMS);
> 
> Why did you change this?

I am getting many multiply defined types when including pr_log.h.  I have noted this and will look at it when I talk to Ethan.

Comment 25

[enda mannion (:emannion)]

Attachment: revised: Add DTLS data to VCMStartICE parameters

Comment 26

[Ethan Hugg [:ehugg]]

>> > +  //PR_ASSERT(idx < CC_MAX_STREAMS);
>> 
>> Why did you change this?
>
>I am getting many multiply defined types when including pr_log.h.  I have noted this and will >look at it when I talk to Ethan.

EKR added NO_NSPR_10_SUPPORT to the #defs in signaling.gyp for the OSX builds to fix this.  Could you get the latest and touch configure.in and try again?

Comment 27

[Eric Rescorla (:ekr)]

(In reply to enda mannion (:emannion) from comment #24)
> > Use sizeof() for the buffer lengths here and below.
> 
> These are #defines  sizeof if not working for me here.  I had tried it
> already.

What problem are you having? sizeof() of a buffer should work.
I do it pretty often.

Comment 28

[enda mannion (:emannion)]

Attachment: revised: Add DTLS data to VCMStartICE parameters

Comment 29

[enda mannion (:emannion)]

(In reply to Eric Rescorla from comment #27)
> (In reply to enda mannion (:emannion) from comment #24)
> > > Use sizeof() for the buffer lengths here and below.
> > 
> > These are #defines  sizeof if not working for me here.  I had tried it
> > already.
> 
> What problem are you having? sizeof() of a buffer should work.
> I do it pretty often.

sizeof will work for me for buffers but not when the #define value is a number.

Comment 30

[enda mannion (:emannion)]

Attachment: revised: Add DTLS data to VCMStartICE parameters

Revised with all comments incorporated.

Comment 31

[Ethan Hugg [:ehugg]]

Comment on attachment 653350 [details] [diff] [review]
revised: Add DTLS data to VCMStartICE parameters

Pushed to Alder - https://hg.mozilla.org/projects/alder/rev/05a978a52d7c