Review Board 1.7.16


Use the correct IP address in the c and o sdp lines when using an externally mapped IP address

Review Request #566 - Created March 16, 2010 and submitted

ebroad
/trunk
17044
Reviewers
asterisk-dev
Asterisk
The issue stems from a call to ast_rtp_instance_get_local_address() in chan_sip.c in get_our_media_address(). ast_rtp_instance_get_local_address() will always return the internal IP address of the Asterisk machine, which could pose a problem when using NAT.

A simple solution would be to override {v,t}sin->sin_addr in get_our_media_address() with p->ourip.sin_addr.s_addr if it is different(which is what the attached diff does), though I don't know what that might break.

Edit 1: This patch short-circuits some logic later on in get_our_media_address(). ast_rtp_instance_get_local_address() does exactly as the function name suggests, it gets the local address of the machine the Asterisk is running on, which is bad for NAT setups. Should we be calling this in get_our_media_address() or instead just use p->ourip? or should we try to stick with letting the RTP engine decide, and modifying ast_rtp_instance_get_local_address() to return our externally mapped IP when appropriate, which would break from what the function is supposed to do? Comments anyone? 
Applied to trunk, compiled, fax and voice calls made to and from Asterisk successfully.

Changes between revision 3 and 4

1 2 3 4
1 2 3 4

  1. /trunk/channels/chan_sip.c: Loading...
/trunk/channels/chan_sip.c
Diff Revision 3 Diff Revision 4
[20] 10259 lines
[+20] [+] static void add_noncodec_to_sdp(const struct sip_pvt *p, int format,
10260
static void get_our_media_address(struct sip_pvt *p, int needvideo, int needtext,
10260
static void get_our_media_address(struct sip_pvt *p, int needvideo, int needtext,
10261
				  struct ast_sockaddr *addr, struct ast_sockaddr *vaddr,
10261
				  struct ast_sockaddr *addr, struct ast_sockaddr *vaddr,
10262
				  struct ast_sockaddr *taddr, struct ast_sockaddr *dest,
10262
				  struct ast_sockaddr *taddr, struct ast_sockaddr *dest,
10263
				  struct ast_sockaddr *vdest, struct ast_sockaddr *tdest)
10263
				  struct ast_sockaddr *vdest, struct ast_sockaddr *tdest)
10264
{
10264
{

    
   
10265
	int use_externip = 0;

    
   
10266

   
10265
	/* First, get our address */
10267
	/* First, get our address */
10266
	ast_rtp_instance_get_local_address(p->rtp, addr);
10268
	ast_rtp_instance_get_local_address(p->rtp, addr);
10267
	if (p->vrtp) {
10269
	if (p->vrtp) {
10268
		ast_rtp_instance_get_local_address(p->vrtp, vaddr);
10270
		ast_rtp_instance_get_local_address(p->vrtp, vaddr);
10269
	}
10271
	}
10270
	if (p->trtp) {
10272
	if (p->trtp) {
10271
		ast_rtp_instance_get_local_address(p->trtp, taddr);
10273
		ast_rtp_instance_get_local_address(p->trtp, taddr);
10272
	}
10274
	}
10273

    
   
10275

   
10274
	/* If our real IP differs from our internal IP, i.e. NAT, use that for media_address if media_address isn't explicitly set */
10276
	/* If our real IP differs from the local address returned by the RTP engine, use it. */
10275
	if ((ast_sockaddr_isnull(&media_address) && ast_sockaddr_cmp_addr(addr, &p->ourip))) {
10277
	/* The premise is that if we are already using that IP to communicate with the client, */
10276
		ast_sockaddr_copy(&media_address, &p->ourip); 
10278
	/* we should be using it for RTP too. */
10277
	}
10279
        use_externip = ast_sockaddr_cmp_addr(&p->ourip, addr);
10278

    
   
10280

   
10279
	/* Now, try to figure out where we want them to send data */
10281
	/* Now, try to figure out where we want them to send data */
10280
	/* Is this a re-invite to move the media out, then use the original offer from caller  */
10282
	/* Is this a re-invite to move the media out, then use the original offer from caller  */
10281
	if (!ast_sockaddr_isnull(&p->redirip)) {	/* If we have a redirection IP, use it */
10283
	if (!ast_sockaddr_isnull(&p->redirip)) {	/* If we have a redirection IP, use it */
10282
		ast_sockaddr_copy(dest, &p->redirip);
10284
		ast_sockaddr_copy(dest, &p->redirip);
[+20] [20] 9 lines
[+20] static void get_our_media_address(struct sip_pvt *p, int needvideo, int needtext,
10292
		 *
10294
		 *
10293
		 * 1. Provided by the RTP engine.
10295
		 * 1. Provided by the RTP engine.
10294
		 */
10296
		 */
10295
		ast_sockaddr_copy(dest,
10297
		ast_sockaddr_copy(dest,
10296
				  !ast_sockaddr_isnull(&media_address) ? &media_address :
10298
				  !ast_sockaddr_isnull(&media_address) ? &media_address :
10297
				  !ast_sockaddr_is_any(addr)           ? addr           :
10299
				  !ast_sockaddr_is_any(addr) && !use_externip ? addr    :
10298
				  &p->ourip);
10300
				  &p->ourip);
10299
		ast_sockaddr_set_port(dest, ast_sockaddr_port(addr));
10301
		ast_sockaddr_set_port(dest, ast_sockaddr_port(addr));
10300
	}
10302
	}
10301

    
   
10303

   
10302
	if (needvideo) {
10304
	if (needvideo) {
[+20] [20] 12 lines
[+20] static void get_our_media_address(struct sip_pvt *p, int needvideo, int needtext,
10315
			 *
10317
			 *
10316
			 * 1. Provided by the RTP engine.
10318
			 * 1. Provided by the RTP engine.
10317
			 */
10319
			 */
10318
			ast_sockaddr_copy(vdest,
10320
			ast_sockaddr_copy(vdest,
10319
					  !ast_sockaddr_isnull(&media_address) ? &media_address :
10321
					  !ast_sockaddr_isnull(&media_address) ? &media_address :
10320
					  !ast_sockaddr_is_any(vaddr)          ? vaddr          :
10322
					  !ast_sockaddr_is_any(vaddr) && !use_externip ? vaddr  :
10321
					  &p->ourip);
10323
					  &p->ourip);
10322
			ast_sockaddr_set_port(vdest, ast_sockaddr_port(vaddr));
10324
			ast_sockaddr_set_port(vdest, ast_sockaddr_port(vaddr));
10323
		}
10325
		}
10324
	}
10326
	}
10325

    
   
10327

   
[+20] [20] 13 lines
[+20] static void get_our_media_address(struct sip_pvt *p, int needvideo, int needtext,
10339
			 *
10341
			 *
10340
			 * 1. Provided by the RTP engine.
10342
			 * 1. Provided by the RTP engine.
10341
			 */
10343
			 */
10342
			ast_sockaddr_copy(tdest,
10344
			ast_sockaddr_copy(tdest,
10343
					  !ast_sockaddr_isnull(&media_address) ? &media_address  :
10345
					  !ast_sockaddr_isnull(&media_address) ? &media_address  :
10344
					  !ast_sockaddr_is_any(taddr)          ? taddr           :
10346
					  !ast_sockaddr_is_any(taddr) && !use_externip ? taddr   :
10345
					  &p->ourip);
10347
					  &p->ourip);
10346
			ast_sockaddr_set_port(tdest, ast_sockaddr_port(taddr));
10348
			ast_sockaddr_set_port(tdest, ast_sockaddr_port(taddr));
10347
		}
10349
		}
10348
	}
10350
	}
10349
}
10351
}
[+20] [20] 18157 lines
  1. /trunk/channels/chan_sip.c: Loading...

https://reviewboard.asterisk.org/ runs on a server provided by Digium, Inc. and uses bandwidth donated to the open source Asterisk community by API Digital Communications in Huntsville, AL USA.
Please report problems with this site to asteriskteam@digium.com.