Summary:	ASTERISK-04006: [patch] Incorrect indenting, possible bug
Reporter:	kleptog (kleptog)	Labels:
Date Opened:	2005-04-27 15:48:31	Date Closed:	2008-06-07 10:59:31
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) zaptel.c.diff
Description:	If you pull the latest head of zaptel/zaptel.c and look in the function zaptel_proc_read there is a section of code incorrectly indented. Also, there is an issue with the routine returning too much data. I noticed it because the proc output was not always newline terminated. ****** ADDITIONAL INFORMATION ****** The incorrectly indented code is in the final for loop, just before the test on ZT_FLAG_OPEN. I think it's line 546. Secondly, the proc routine doesn't handle partial reads. So when you cat the /proc file, it gets truncated at 1024 bytes. Using strace: # strace -e read cat /proc/zaptel/3           read(3, "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\3\0\3\0\1\0\0\0\30\222"..., 1024) = 1024 read(3, "Span 3: TE4/0/3 \"TE410P (PCI) Ca"..., 1024) = 1024 read(3, "", 1024)                       = 0 Whereas using wc gives: # strace -e read wc /proc/zaptel/3         read(3, "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\3\0\3\0\1\0\0\0\30\222"..., 1024) = 1024 read(3, "Span 3: TE4/0/3 \"TE410P (PCI) Ca"..., 16384) = 1094 read(3, "", 16384)       As you can see, the proc routine doesn't handle the first read not returning all data. There is a comment there about not returning more than 1K of data on a 2.6 kernel, but in this case there is too much. Linux Kernel 2.6.8
Comments:	By: twisted (twisted) 2005-05-10 23:54:58 as far as the formatting goes, that's fine and can be fixed easily, feel free to submit a formatting patch. As far as your proc issue goes, I was able to get it to return 16384, but it did pull a full read each time.. using the exact methods you used. I'm not sure exactly sure what is going on here.  Kernel on my side is 2.6.9. By: Kevin P. Fleming (kpfleming) 2005-05-14 22:44:36 Can you still reproduce this problem? By: kleptog (kleptog) 2005-05-15 09:38:18 Sure, this is reproducable. The function zaptel_proc_read() simply doesn't handle the case where off != 0. The fact that most people don't have enough channels to trigger it (on a 30 channel E1 only two lines and a few characters are missing) means that the vast majority of people will never see it. It probably depends on exact version of the C library and/or cat binary. The code never checks to see if it exceeded the limit of the buffer passed to it, so it'd probably count as a buffer overflow. According to my copy of Linux Device Drivers you'll have a 4k (PAGE_SIZE) buffer so you're saved by the fact that there's a limit of 31 channels per span. Anyway, I took a leaf out the LDD book and have attached a patch that should fix these issues. It's a simple procedure but may cause slight glitches in the output if the status of the span changes between the two read calls. Also note it is *UNTESTED*. I don't have any Digium hardware on machines I can actually play with. By: Kevin P. Fleming (kpfleming) 2005-05-15 11:54:46 If I'm not mistaken, wasn't the 'seq_file' stuff added to the kernel specifically to combat this sort of problem? By: kleptog (kleptog) 2005-05-15 12:30:00 It was, but I presume the driver should still work on 2.4 and I think maintaining two different versions is a bit silly. By: Kevin P. Fleming (kpfleming) 2005-05-15 13:02:41 Yes, that's a valid point... I wasn't sure whether seq_file was 2.6 only or not. I don't have the appropriate hardware in my system here to test it either, so would you mind posting a note to asterisk-dev asking for someone with an E1 span to test your patch? Thanks. By: kleptog (kleptog) 2005-05-15 14:29:41 Actually, I found a simpler way to reproduce it for anybody with anything zaptel. Like this: perl -e 'open(FH,"/proc/zaptel/1"); seek FH,1,0; print <FH>' It opens the first span, seeks to offset one and reads. Before patch, the output is nothing. After patch it should just work normally. The tip to buzz asterisk-dev to test is good, but I need to suscribe first. By: Clod Patry (junky) 2005-06-20 19:03:05 Any development here? kleptog: is that still a problem? I've digium hardware to test if necessary, patch is still against current HEAD? By: Mark Spencer (markster) 2005-07-31 17:12:47 Fixed in CVS head ,thanks! By: Russell Bryant (russell) 2005-08-04 20:02:28 already fixed this one in 1.0 By: Digium Subversion (svnbot) 2008-06-07 10:59:31 Repository: dahdi Revision: 713 U   trunk/zaptel.c ------------------------------------------------------------------------ r713 | markster | 2008-06-07 10:59:30 -0500 (Sat, 07 Jun 2008) | 2 lines Fix cat /proc/zaptel/foo bug (bug ASTERISK-4006) ------------------------------------------------------------------------ http://svn.digium.com/view/dahdi?view=rev&revision=713