Summary:ASTERISK-04006: [patch] Incorrect indenting, possible bug
Reporter:kleptog (kleptog)Labels:
Date Opened:2005-04-27 15:48:31Date Closed:2008-06-07 10:59:31
Versions:Frequency of
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.


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)