[Home]

Summary:ASTERISK-14303: [patch] incorrect comparation in ast_monitor_change_fname() leads to deletion of recorded files
Reporter:caspy (caspy)Labels:
Date Opened:2009-06-11 04:20:38Date Closed:2009-11-04 18:01:51.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Resources/res_monitor
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 20091029__issue15313__1.4.diff.txt
( 1) 20091103__issue15313__1.4.diff.txt
Description:In res_monitor.c in ast_monitor_change_fname() is done a comparation of new monitor's filename with a current one to preserve of setting chan->monitor->filename_changed in case of file name is not changed.
There is a mistake in filename preparation routines, which makes wrong comparation of the SAME filename: 'chan->monitor->filename_base' contains only current name of file and 'tmpstring' contains the SAME filename but with full path.

In attached patch there is a couple of new debug messages, and a quick fix.
This fix is working ok for me, but i'm not sure it is correct in idea. So, please, review it before commiting.

Here is an example of debug output for this issue:
$ grep CASPY /var/log/asterisk/debug | tail -2
[Jun 11 12:39:39] DEBUG[19438] res_monitor.c: CASPY: ast_monitor_change_fname(): fname_base: in-20090611-123450-4957874222-1244709290-28095, filename_base: in-20090611-123450-4957874222-1244709290-28095
[Jun 11 12:39:39] DEBUG[19438] res_monitor.c: CASPY: ast_monitor_change_fname(): comparing: /var/spool/asterisk/monitor/in-20090611-123450-4957874222-1244709290-28095 and in-20090611-123450-4957874222-1244709290-28095

Here you can see, that filename is the same, but one of compared with a full path. This leads to later deletion of recorded files.
Comments:By: Eliel Sardanons (eliel) 2009-06-11 09:43:33

Thanks for the patch,

Please remove the debug lines added by you ("CASPY: ...").
By: caspy (caspy) 2009-06-11 09:49:38

here it is: res_monitor.c.bug15313.patch
By: caspy (caspy) 2009-06-25 11:06:29

eliel,
is it ok?
By: Leif Madsen (lmadsen) 2009-07-13 10:30:51

Issue is ready for review.
By: Tilghman Lesher (tilghman) 2009-09-01 02:56:56

There is still a case that this misses.  If the new filename is a relative path, containing at least one slash, it is possible for the 2 filenames to be equivalent, yet not compare equivalently.  It may be better to use realpath(3) to resolve these cases for both files and compare the results.
By: caspy (caspy) 2009-09-02 05:53:59

unfortunately, i'm not so good in programming. may be some real developers can help this?
By: Leif Madsen (lmadsen) 2009-09-22 08:55:31

Changing this to confirmed. Either another developer will get to this when it reaches a high enough priority, or the original reporter can keep trying and learning in the process :)
By: Tilghman Lesher (tilghman) 2009-10-22 18:22:20

New patch uploaded that does a complete comparison of two filenames to verify if they are equivalent.
By: Tilghman Lesher (tilghman) 2009-10-27 15:10:06

caspy:  I need testing and feedback on this patch or this issue will be closed.
By: caspy (caspy) 2009-10-28 11:16:24

i'm on holidays till 1st Nov.
as soon as i return, i'll test it.
thanks.
By: caspy (caspy) 2009-11-03 00:14:44.000-0600

patch installed. i'll report results in a few days.
By: caspy (caspy) 2009-11-03 02:33:51.000-0600

something is going wrong, cause files are deleted.
By: caspy (caspy) 2009-11-03 02:56:14.000-0600

tilghman,

i've added a logging like this just before changing filename:

 ast_log(LOG_WARNING, "Changing monitor file name of channel %s from %s to %s\n", chan->name, chan->monitor->filename_base, tmpstring);
 ast_copy_string(chan->monitor->filename_base, tmpstring, sizeof(chan->monitor->filename_base));
 /* chan->monitor->filename_changed = 1; */


and result is:

[Nov  3 11:40:48] WARNING[23274] res_monitor.c: Changing monitor file name of channel DAHDI/4-1 from in-20091103-113853-4951111111-1257237533-214 to /var/spool/asterisk/monitor/in-20091103-113853-4951111111-1257237533-214


current filename in chan->monitor->filename_base contains filename without path, so testing is done in /tmp (which is a workdir setted by safe_asterisk).
but tmpstring contains path, thus testing for unique naming is going wrong.
By: Jeff Peeler (jpeeler) 2009-11-03 13:24:32.000-0600

I hope the problem you're still experiencing is a result of initially setting the filename in the dialplan. I've updated the patch to append the path to the requested fname_base. EDIT: actually, this should be solved for the manager action too.


By: Tilghman Lesher (tilghman) 2009-11-04 10:59:38.000-0600

jpeeler:  perhaps we should change that debug line to an ast_debug, so it can remain there for commit.
By: Jeff Peeler (jpeeler) 2009-11-04 11:03:02.000-0600

Yeah, that's probably a good idea.
By: Digium Subversion (svnbot) 2009-11-04 17:52:36.000-0600

Repository: asterisk
Revision: 227944

U   branches/1.4/res/res_monitor.c

------------------------------------------------------------------------
r227944 | jpeeler | 2009-11-04 17:52:36 -0600 (Wed, 04 Nov 2009) | 14 lines

Fix incorrect filename comparsion after monitor file change

The logic to detect if a requested file is indeed a different file from the
current file was incorrect. The main issue being confusion of the use of
filename_base which was previously set without pathing information and then
compared to another full path. Robust file comparison logic has been added
to properly check if two files are the same even if symlinks are used.

(closes issue ASTERISK-14303)
Reported by: caspy
Patches:
     20091103__issue15313__1.4.diff.txt uploaded by jpeeler (license 325)
     but mostly tilghman's work

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

http://svn.digium.com/view/asterisk?view=rev&revision=227944
By: Digium Subversion (svnbot) 2009-11-04 17:56:27.000-0600

Repository: asterisk
Revision: 227945

_U  trunk/
U   trunk/res/res_monitor.c

------------------------------------------------------------------------
r227945 | jpeeler | 2009-11-04 17:56:26 -0600 (Wed, 04 Nov 2009) | 21 lines

Merged revisions 227944 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
 r227944 | jpeeler | 2009-11-04 17:47:08 -0600 (Wed, 04 Nov 2009) | 14 lines
 
 Fix incorrect filename comparsion after monitor file change
 
 The logic to detect if a requested file is indeed a different file from the
 current file was incorrect. The main issue being confusion of the use of
 filename_base which was previously set without pathing information and then
 compared to another full path. Robust file comparison logic has been added
 to properly check if two files are the same even if symlinks are used.
 
 (closes issue ASTERISK-14303)
 Reported by: caspy
 Patches:
       20091103__issue15313__1.4.diff.txt uploaded by jpeeler (license 325)
       but mostly tilghman's work
........

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

http://svn.digium.com/view/asterisk?view=rev&revision=227945
By: Digium Subversion (svnbot) 2009-11-04 17:57:38.000-0600

Repository: asterisk
Revision: 227946

_U  branches/1.6.0/
U   branches/1.6.0/res/res_monitor.c

------------------------------------------------------------------------
r227946 | jpeeler | 2009-11-04 17:57:38 -0600 (Wed, 04 Nov 2009) | 28 lines

Merged revisions 227945 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r227945 | jpeeler | 2009-11-04 17:50:59 -0600 (Wed, 04 Nov 2009) | 21 lines
 
 Merged revisions 227944 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r227944 | jpeeler | 2009-11-04 17:47:08 -0600 (Wed, 04 Nov 2009) | 14 lines
   
   Fix incorrect filename comparsion after monitor file change
   
   The logic to detect if a requested file is indeed a different file from the
   current file was incorrect. The main issue being confusion of the use of
   filename_base which was previously set without pathing information and then
   compared to another full path. Robust file comparison logic has been added
   to properly check if two files are the same even if symlinks are used.
   
   (closes issue ASTERISK-14303)
   Reported by: caspy
   Patches:
         20091103__issue15313__1.4.diff.txt uploaded by jpeeler (license 325)
         but mostly tilghman's work
 ........
................

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

http://svn.digium.com/view/asterisk?view=rev&revision=227946
By: Digium Subversion (svnbot) 2009-11-04 17:59:11.000-0600

Repository: asterisk
Revision: 227947

_U  branches/1.6.2/
U   branches/1.6.2/res/res_monitor.c

------------------------------------------------------------------------
r227947 | jpeeler | 2009-11-04 17:59:11 -0600 (Wed, 04 Nov 2009) | 28 lines

Merged revisions 227945 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r227945 | jpeeler | 2009-11-04 17:50:59 -0600 (Wed, 04 Nov 2009) | 21 lines
 
 Merged revisions 227944 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r227944 | jpeeler | 2009-11-04 17:47:08 -0600 (Wed, 04 Nov 2009) | 14 lines
   
   Fix incorrect filename comparsion after monitor file change
   
   The logic to detect if a requested file is indeed a different file from the
   current file was incorrect. The main issue being confusion of the use of
   filename_base which was previously set without pathing information and then
   compared to another full path. Robust file comparison logic has been added
   to properly check if two files are the same even if symlinks are used.
   
   (closes issue ASTERISK-14303)
   Reported by: caspy
   Patches:
         20091103__issue15313__1.4.diff.txt uploaded by jpeeler (license 325)
         but mostly tilghman's work
 ........
................

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

http://svn.digium.com/view/asterisk?view=rev&revision=227947
By: Digium Subversion (svnbot) 2009-11-04 18:01:47.000-0600

Repository: asterisk
Revision: 227948

_U  branches/1.6.1/
U   branches/1.6.1/res/res_monitor.c

------------------------------------------------------------------------
r227948 | jpeeler | 2009-11-04 18:01:47 -0600 (Wed, 04 Nov 2009) | 28 lines

Merged revisions 227945 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r227945 | jpeeler | 2009-11-04 17:50:59 -0600 (Wed, 04 Nov 2009) | 21 lines
 
 Merged revisions 227944 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r227944 | jpeeler | 2009-11-04 17:47:08 -0600 (Wed, 04 Nov 2009) | 14 lines
   
   Fix incorrect filename comparsion after monitor file change
   
   The logic to detect if a requested file is indeed a different file from the
   current file was incorrect. The main issue being confusion of the use of
   filename_base which was previously set without pathing information and then
   compared to another full path. Robust file comparison logic has been added
   to properly check if two files are the same even if symlinks are used.
   
   (closes issue ASTERISK-14303)
   Reported by: caspy
   Patches:
         20091103__issue15313__1.4.diff.txt uploaded by jpeeler (license 325)
         but mostly tilghman's work
 ........
................

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

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