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.

Diff revision 4 (Latest)

1 2 3 4
1 2 3 4

  1. /trunk/channels/chan_sip.c: Loading...
/trunk/channels/chan_sip.c
Revision 281293 New Change
[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

   

    
   
10276
	/* If our real IP differs from the local address returned by the RTP engine, use it. */

    
   
10277
	/* The premise is that if we are already using that IP to communicate with the client, */

    
   
10278
	/* we should be using it for RTP too. */

    
   
10279
        use_externip = ast_sockaddr_cmp_addr(&p->ourip, addr);

    
   
10280

   
10274
	/* 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 */
10275
	/* 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  */
10276
	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 */
10277
		ast_sockaddr_copy(dest, &p->redirip);
10284
		ast_sockaddr_copy(dest, &p->redirip);
10278
	} else {
10285
	} else {
[+20] [20] 8 lines
[+20] static void get_our_media_address(struct sip_pvt *p, int needvideo, int needtext,
10287
		 *
10294
		 *
10288
		 * 1. Provided by the RTP engine.
10295
		 * 1. Provided by the RTP engine.
10289
		 */
10296
		 */
10290
		ast_sockaddr_copy(dest,
10297
		ast_sockaddr_copy(dest,
10291
				  !ast_sockaddr_isnull(&media_address) ? &media_address :
10298
				  !ast_sockaddr_isnull(&media_address) ? &media_address :
10292
				  !ast_sockaddr_is_any(addr)           ? addr           :
10299
				  !ast_sockaddr_is_any(addr) && !use_externip ? addr    :
10293
				  &p->ourip);
10300
				  &p->ourip);
10294
		ast_sockaddr_set_port(dest, ast_sockaddr_port(addr));
10301
		ast_sockaddr_set_port(dest, ast_sockaddr_port(addr));
10295
	}
10302
	}
10296

    
   
10303

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

    
   
10327

   
[+20] [20] 13 lines
[+20] static void get_our_media_address(struct sip_pvt *p, int needvideo, int needtext,
10334
			 *
10341
			 *
10335
			 * 1. Provided by the RTP engine.
10342
			 * 1. Provided by the RTP engine.
10336
			 */
10343
			 */
10337
			ast_sockaddr_copy(tdest,
10344
			ast_sockaddr_copy(tdest,
10338
					  !ast_sockaddr_isnull(&media_address) ? &media_address  :
10345
					  !ast_sockaddr_isnull(&media_address) ? &media_address  :
10339
					  !ast_sockaddr_is_any(taddr)          ? taddr           :
10346
					  !ast_sockaddr_is_any(taddr) && !use_externip ? taddr   :
10340
					  &p->ourip);
10347
					  &p->ourip);
10341
			ast_sockaddr_set_port(tdest, ast_sockaddr_port(taddr));
10348
			ast_sockaddr_set_port(tdest, ast_sockaddr_port(taddr));
10342
		}
10349
		}
10343
	}
10350
	}
10344
}
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.