xtknight / mt7610u-linksys-ae6000-wifi-fixes

Trying to fix the MT7610U chipset driver by MediaTek so it's usable on modern Linux kernels and with nl80211/NetworkManager (and not completely breaking on old ones, maybe...)
187 stars 71 forks source link

Fixed a compile error. #29

Closed Dumonu closed 6 years ago

Dumonu commented 7 years ago

On line 696 of os/linux/sta_ioctl.c, there was a call to memcpy that used &addr for the src location. This caused an error because addr is a pointer to a struct array, and therefore it would try to copy junk data from the location of the pointer rather than from the array itself.

Dumonu commented 7 years ago

Actually, I've noticed that the 2nd line underneath my change is very similar to the one I changed. In addition, it also has the &. However, this version compiles and works fine. I wonder if it has to do with the difference between declaring on the heap and on the stack. But I'm pretty sure both addr and qual are pointers, so I'm confused as to why the compiler isn't yelling about this second line.

xtknight commented 7 years ago

@Dumonu Is commit 3235bed supposed to be addressing a compile error? I actually don't see any compile error or warning occur in sta_ioctl.c when compiling on Kernel 4.14-rc4 and gcc 5.4 at least. Linux version 4.14.0-rc4-mk (root@andy-home) (gcc version 5.4.0 20160609 (Ubuntu 5.4.0-6ubuntu1~16.04.4)) #1 SMP Fri Oct 13 01:28:27 KST 2017

Could you clarify your gcc and kernel version and the compile error text?

I can't say I have a lot of experience with this code but the memcpy on qual doesn't look right to me, either. "/ signal quality present (sort of) /". Maybe that's what they meant by "sort of" :) Seems like the use case for this function is SIOCGIWAPLIST which lists peers/APs, which is very core code. It's probably worth a look.

Probably worth going through this whole code base with static code analysis too, which I wonder when I'll ever get to. Quick run-through with cppcheck-gui on the directory seems informative though.

Dumonu commented 7 years ago

I've rebooted into my Windows install, so I can't answer exactly right now, I'll get back to you on that. But when I updated my Linux install today, then made sure this driver was working by recompiling, I got an error that said, roughly, that memcpy was copying more than the pointer was actually pointing to. Or something. An odd error, given that I thought memcpy was supposed to just copy whatever's there without caring. Removing the & made that error stop happening. And it makes more sense to pass a pointer than a pointer to a pointer to memcpy anyways.

nfode commented 7 years ago

Using Manjaro with Kernel 4.13 this pull request fixed the problem and I could install the driver.

Dumonu commented 7 years ago

Sorry about taking so long to reply. I had completely forgotten about this until nfode replied to it. I am running Arch Linux kernel version 4.13.7-1-ARCH x86_64. I am on gcc version 7.2.0. Here is the error that make throws when the argument is &addr:

In function ‘memcpy’,
    inlined from ‘rt_ioctl_iwaplist’ at /root/mt7610u-linksys-ae6000-wifi-fixes/os/linux/../../os/linux/sta_ioctl.c:696:2:
./include/linux/string.h:305:4: error: call to ‘__read_overflow2’ declared with attribute error: detected read beyond size of object passed as 2nd parameter
    __read_overflow2();
    ^~~~~~~~~~~~~~~~~~
xtknight commented 6 years ago

Apparently the other pull request was about the same thing on the same line of code. I merged that one, so I guess it's safe to close this one. Thanks for reporting this!

Dumonu commented 6 years ago

OK. That works for me.