AP_RangeFinder: Possible NULL pointer dereference#8783
AP_RangeFinder: Possible NULL pointer dereference#8783KimHyungSub wants to merge 3 commits intoArduPilot:masterfrom KimHyungSub:master
Conversation
Possible NULL pointer dereference in libraries/AP_RangeFinder/AP_RangeFinder_BBB_PRU.cpp
|
Wow. This is a doozy - well spotted. There are several problems here which your PR doesn't address. Most notably memfd isn't checked for being -1, ctrl for and ram being nullptr. Additionally, your fix may mean file descriptors are leaked in the case of a failed read. There are enough resources involved here that I think a (minor cleanup at the same time: don't pre-declare variables at the top, declare when defining, ie.e. Are you happy to chase this more-complete cleanup down? |
|
@KimHyungSub, it's great that you're finding these issues! nicely done! |
|
@peterbarker Ah! you are right! I ignore other parts without the file descriptor. |
#8783 Deal with *memfd*, *ctrl*
| ram = mmap(0, PRU0_IRAM_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, mem_fd, PRU0_IRAM_BASE); | ||
|
|
||
| uint32_t mem_fd = open("/dev/mem", O_RDWR | O_SYNC | O_CLOEXEC); | ||
| if(mem_fd == -1) |
There was a problem hiding this comment.
style-guide requires that the brace goes on the same line as the if, and a space between the if and the pren.
| uint32_t mem_fd = open("/dev/mem", O_RDWR | O_SYNC | O_CLOEXEC); | ||
| if(mem_fd == -1) | ||
| { | ||
| return false; |
There was a problem hiding this comment.
For consistency, create a label for this case, too.
| uint32_t *ctrl = (uint32_t*)mmap(0, 0x1000, PROT_READ | PROT_WRITE, MAP_SHARED, mem_fd, PRU0_CTRL_BASE); | ||
| if(ctrl == nullptr) | ||
| { | ||
| goto fail_mmap; |
| void *ram = mmap(0, PRU0_IRAM_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, mem_fd, PRU0_IRAM_BASE); | ||
| if(ram == nullptr) | ||
| { | ||
| goto fail_mmap; |
| } | ||
|
|
||
| if(fread(ram, PRU0_IRAM_SIZE, 1, file) != 1) | ||
There was a problem hiding this comment.
Extraneous whitespace; see if you can turn on "show trailing whitespace" in your editor.
|
|
||
| if(fread(ram, PRU0_DRAM_SIZE, 1, file) != 1) | ||
| if(fread(ram, PRU0_DRAM_SIZE, 1, file) != 1) |
There was a problem hiding this comment.
Watch out for whitespace changes. Esp when it's stuffing the indent up :-)
| @@ -51,14 +51,25 @@ AP_RangeFinder_BBB_PRU::AP_RangeFinder_BBB_PRU(RangeFinder::RangeFinder_State &_ | |||
| bool AP_RangeFinder_BBB_PRU::detect() | |||
| { | |||
| bool result = true; | |||
There was a problem hiding this comment.
Won't need result any more.
| rangerpru = (volatile struct range*)ram; | ||
|
|
||
| return result; | ||
| return result; |
There was a problem hiding this comment.
Can return true here... also, whitespace :-)
peterbarker
left a comment
There was a problem hiding this comment.
Factoring out a load_file_at_address(...) could be nice here, and cut down on the failure cases you need to worry about.
| goto FAIL_MEM_FD; | ||
| } | ||
|
|
||
| uint32_t *ctrl = (uint32_t*)mmap(0, 0x1000, PROT_READ | PROT_WRITE, MAP_SHARED, mem_fd, PRU0_CTRL_BASE); |
There was a problem hiding this comment.
This must be unmapped in the relevant failure cases.
| uint32_t *ctrl; | ||
| void *ram; | ||
| uint32_t mem_fd = open("/dev/mem", O_RDWR | O_SYNC | O_CLOEXEC); | ||
| if(mem_fd == -1) { |
| @@ -83,14 +87,12 @@ bool AP_RangeFinder_BBB_PRU::detect() | |||
|
|
|||
There was a problem hiding this comment.
return value of this mmap isn't checked
| { | ||
| result = false; | ||
| if(file == nullptr) { | ||
| goto FAIL_FOPEN; |
There was a problem hiding this comment.
You will need a separate label for this. FAIL_FOPEN_DATA, perhaps?
|
This is bitrooting and low on safety as it touch only the PRU code. I will close this. Feel free to reopen with the requested changes. thanks anyway |
Issues: #8764
I correct these issues and then open a pull request.
Please check if the code is correct or not.