[Home]

Summary:ASTERISK-01046: [patch] mysql-friends times out after 120 seconds. Also memory leak.
Reporter:z_smurf (z_smurf)Labels:
Date Opened:2004-02-17 19:49:44.000-0600Date Closed:2008-01-15 14:44:01.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) bug.proof.txt
( 1) mysql_friend_patch.diff.txt
( 2) mysql_friend_patch2.diff.txt
( 3) mysql_friend_patch3.diff.txt
( 4) mysql_friend_patch4.diff.txt
( 5) mysql_friend_patch5_diff.txt
( 6) wrong_place_diff.TXT
Description:Peers defined as mysql_friends in database (instead of sip.conf) has serious problems in two ways.

1) They MUST register every 120 second.
2) If not, there is a memory leak when dialling a peer.

****** ADDITIONAL INFORMATION ******

Because there is not sent any OPTIONS-packets to mysql-friend-peers to see if they are alive, the timestamp will only be updated on REGISTER events.

This will happen when calling a peer that has timed out:
* call from app_dial to channel.c which query then chan_sip.c for the peer. In chan_sip.c this will happen:
* *sip_request( peer name) calls create_addr.
* create_addr( peer name) calls mysql_peer.
* mysql_peer( peer name) allocates memory for a temporary peer, reads the peer from database, finds out that it has expired, empties the IP and PORT but returns secessfully.
* Back in create_addr the peer is threated as found, BUT suddenly there is a check of valid adresses. Because this is false, the pointer of our temporary peer is set to NULL. It also looses control that this was a temporary peer, so the free will not be called. return is -1.
* back in sip_request, return is NULL
* back in ast_request, the same NULL is returned.

Summary: The call failed, some memory is lost, and console shows this:
Feb 18 01:04:55 NOTICE[23571]: app_dial.c:528 dial_exec: Unable to create channel of type 'SIP'

Bigfix (attached) fixes this:
* Increase time-check to be at least two hours.
* Fix so the peer is freed if its a temp-peer.
Comments:By: z_smurf (z_smurf) 2004-02-18 19:14:08.000-0600

Found another bug in mysql_friend:

A peer cannot actually register (change registration-time) if it has not expired.

The applied patch fixes this. It actually changes the meaning of the database column "regseconds" from being the timestamp of a registration to be the timestamp of when a peer expires.

With the patch, both REGISTER and UNREGISTER works, as the timestamp is set to current-time if the peer does an unregister.
By: z_smurf (z_smurf) 2004-02-19 21:45:11.000-0600

mysql_friend_patch3.diff.txt:
Included the fix from bug ASTERISK-1021063, so this patch now fixes many important errors.
By: Rob Gagnon (rgagnon) 2004-02-20 02:28:29.000-0600

Question:  Any special reason the numerical values of the SQL statements are wrapped with quotes in chan_sip.c??

I would think you wouldn't need them for "port" or "regseconds" due to the fact they are numbers.

For instance:
"UPDATE sipfriends SET ipaddr=\"%s\", port=\"%d\", regseconds=\"%ld\" WHERE name=\"%s\""

should be:
"UPDATE sipfriends SET ipaddr=\"%s\", port=%d, regseconds=%ld WHERE name=\"%s\""

edited on: 02-20-04 01:17
By: z_smurf (z_smurf) 2004-02-20 05:31:36.000-0600

rgagnon:
First, if port is null, it would be better to write "" instead of nothing.
Second, MySql is not as stupid as oracle. Mysql transforms these strings to numbers, and does not slow down a bit because of this. Putting variables in quotes is a good way to make the SQL more human readable.
By: Mark Spencer (markster) 2004-02-20 10:56:00.000-0600

I believe I fixed it in CVS, although I did it a little differently.  Anyway if you still have trouble, please reopen the bug and get it labeled "major" so it can be fixed before 1.0.
By: z_smurf (z_smurf) 2004-02-22 16:12:41.000-0600

Reopening bug.

The problem still occur. If there is no comunication between peer and * in 120 seconds, phone will become unreacheable until it registers again.

Proof: I have connected 4 GS-phones to todays CVS. This is the output from Database. The last column sould never be less than -120, but it does.

Please take another look at my attached patch. There is three good benefits:

1) It does only write to dagabase when phone does a *register*. Your way, there is a write every 120 seconds (and if that fails, the phone becomes unavailable).
2) There is no need for a peer to re-register even if it becomes unavaiable for a short while. The registration last as long the phone requests (but does not exceed DEFAULT_EXPIRY_LIMIT).
3) The phone can unregister. That does not work now.

I will upload my patch again, altered to match the latest CVS.
By: Mark Spencer (markster) 2004-02-22 17:22:30.000-0600

The "Expires" of the register should say when the entry expires, and we should follow it.  If we're ignoring the "expires" value then we need to do it properly.  if they're not registering quickly enough, that's their bug.  Am I missing something?
By: z_smurf (z_smurf) 2004-02-22 18:40:55.000-0600

I think you missed this in the bugfix:

               snprintf(query, sizeof(query), "UPDATE sipfriends SET ipaddr=\"%s\", port=\"%d\", regseconds=\"%ld\", username=\"%s\" WHERE name=\"%s\"",
-                       inet_ntoa(sin->sin_addr), ntohs(sin->sin_port), nowtime, uname, name);
+                       inet_ntoa(sin->sin_addr), ntohs(sin->sin_port), nowtime + expiry, uname, name);

Maybe it sould read "expires" instead of expiry, I am not too sure of the meaning. Anyhow, the variable should contain the registration-expire-time the phone sends when it registers.

The current CVS does not keep peers available.
Phones think they are registered, and thec can place calls, randomly they cannot take calls.

edited on: 02-22-04 17:27
By: Mark Spencer (markster) 2004-02-22 21:17:15.000-0600

You're right, I totally missed that.

I can't get the attached one to apply to CVS, though.  Can you double check it?
By: z_smurf (z_smurf) 2004-02-23 16:35:01.000-0600

Sorry.
mysql_friend_patch5_diff.txt applies.
By: Rob Gagnon (rgagnon) 2004-02-23 17:00:09.000-0600

also appears to solve bug ASTERISK-8900935

edited on: 02-24-04 11:10
By: Malcolm Davenport (mdavenport) 2004-02-24 15:01:40.000-0600

Changes made to CVS.  Thank you :)
By: z_smurf (z_smurf) 2004-02-24 19:14:36.000-0600

Oops!
You got the mysql_update_peer() wrong in CVS. Now we are updating database with autocreate-peers instead of mysql-peers :-)

I'll apply a patch "wrong_place.diff.txt".
By: Malcolm Davenport (mdavenport) 2004-02-24 20:28:27.000-0600

Got it this time...I'm sure of it. ;)
By: Digium Subversion (svnbot) 2008-01-15 14:44:00.000-0600

Repository: asterisk
Revision: 2202

U   trunk/channels/chan_sip.c
U   trunk/contrib/scripts/sip-friends.sql

------------------------------------------------------------------------
r2202 | markster | 2008-01-15 14:43:59 -0600 (Tue, 15 Jan 2008) | 2 lines

Improve SIP friends support (should address bugs ASTERISK-1057 & ASTERISK-1046)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=2202
By: Digium Subversion (svnbot) 2008-01-15 14:44:01.000-0600

Repository: asterisk
Revision: 2203

U   branches/v1-0_stable/channels/chan_sip.c
U   branches/v1-0_stable/contrib/scripts/sip-friends.sql

------------------------------------------------------------------------
r2203 | markster | 2008-01-15 14:44:00 -0600 (Tue, 15 Jan 2008) | 2 lines

Fix some SIP friends issues (bug ASTERISK-1057 & ASTERISK-1046)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=2203