You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/04/22 00:31:52 UTC

[GitHub] [incubator-nuttx] masayuki2009 opened a new pull request #843: Fix heap corruption

masayuki2009 opened a new pull request #843:
URL: https://github.com/apache/incubator-nuttx/pull/843


   ## Description
   
   **Describe problem solved by the PR**
   
   During testing maix-bit:kostest, I found a heap corruption which destroys USR_HEAP->mm_delaylist which was introduced in https://github.com/apache/incubator-nuttx/pull/761. This PR fixes this heap corruption.
   
   **Describe your solution**
   
   ```struct mm_delaynode_s *mm_delaylist``` is included in CONFIG_BUILD_PROTECTED. Actually, it is not accessed from userland but from kernel in protected build mode. However, ```sizeof(struct mm_heap_s)``` must be correct in userland to avoid heap corruption by accessing to adjacent memory area.
   
   **Describe possible alternatives**
   
   I have no ideas so far.
   
   **Additional context**
   
   Please see https://github.com/apache/incubator-nuttx/pull/761 as well.
   
   ## Type of change
   
   - [x] Bug fix (non-breaking change which fixes an issue)
   
   ## How Has This Been Tested?
   
   I tested this PR with maix-bit:kostst on qemu.
   The instructions are also updated in this PR.
   
   **Test Configuration**:
   
   * Nuttx board/config: maix-bit:kostest, maix-bit:smp
   * Hardware: qemu-system-riscv64
   * Toolchain: riscv64-unknown-elf-gcc 8.3.0
   
   ## Checklist:
   
   - [x] My code follows the style guidelines of this project (NEED link to how to run checkpatch)
   - [x] I have performed a self-review of my own code
   - [ ] I have commented my code, particularly in hard-to-understand areas
   - [x] I have made corresponding changes to the documentation
   - [x] My changes generate no new warnings
   - [ ] I have added tests that prove my fix is effective or that my feature works
   - [ ] New and existing unit tests pass locally with my changes
   - [ ] Any dependent changes have been merged and published in downstream modules
   - [ ] I have checked my code and corrected any misspellings (NEED link to how to run checkpatch spelling)
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #843: Fix heap corruption

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #843:
URL: https://github.com/apache/incubator-nuttx/pull/843#discussion_r412651429



##########
File path: include/nuttx/mm/mm.h
##########
@@ -258,7 +259,8 @@ struct mm_heap_s
 
   struct mm_freenode_s mm_nodelist[MM_NNODES];
 
-#if defined(CONFIG_BUILD_FLAT) || defined(__KERNEL__)
+#if defined(CONFIG_BUILD_FLAT) || defined(CONFIG_BUILD_PROTECTED) \
+  || defined(__KERNEL__)

Review comment:
       Actually the initial code don't have #ifdef/#endif, but @liguiding found that up_interrupt_context/enter_critical_section/leave_critical_section doesn't expose to userspace. That is why you see this guard in the mainline. Since the userspace should never call kmm_free in the interrupt context, it's good to remove the delay free related code for userspace library until you report this issue. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #843: Fix heap corruption

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #843:
URL: https://github.com/apache/incubator-nuttx/pull/843#discussion_r412651429



##########
File path: include/nuttx/mm/mm.h
##########
@@ -258,7 +259,8 @@ struct mm_heap_s
 
   struct mm_freenode_s mm_nodelist[MM_NNODES];
 
-#if defined(CONFIG_BUILD_FLAT) || defined(__KERNEL__)
+#if defined(CONFIG_BUILD_FLAT) || defined(CONFIG_BUILD_PROTECTED) \
+  || defined(__KERNEL__)

Review comment:
       Actually the initial code don't have #ifdef/#endif, but @liguiding found that up_interrupt_context/enter_critical_section/leave_critical_section doesn't expose to userspace. That is why you see this guard in the mainline. Since the userspace should never call kmm_free in the interrupt context, it's reasonable to remove the delay free related code from userspace library until you report this issue. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] masayuki2009 commented on a change in pull request #843: Fix heap corruption

Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #843:
URL: https://github.com/apache/incubator-nuttx/pull/843#discussion_r412632781



##########
File path: include/nuttx/mm/mm.h
##########
@@ -258,7 +259,8 @@ struct mm_heap_s
 
   struct mm_freenode_s mm_nodelist[MM_NNODES];
 
-#if defined(CONFIG_BUILD_FLAT) || defined(__KERNEL__)
+#if defined(CONFIG_BUILD_FLAT) || defined(CONFIG_BUILD_PROTECTED) \
+  || defined(__KERNEL__)

Review comment:
       @xiaoxiang781216 Thanks for your comments. I will modify the code and push it later.
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] masayuki2009 commented on a change in pull request #843: Fix heap corruption

Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #843:
URL: https://github.com/apache/incubator-nuttx/pull/843#discussion_r412668158



##########
File path: include/nuttx/mm/mm.h
##########
@@ -258,7 +259,8 @@ struct mm_heap_s
 
   struct mm_freenode_s mm_nodelist[MM_NNODES];
 
-#if defined(CONFIG_BUILD_FLAT) || defined(__KERNEL__)
+#if defined(CONFIG_BUILD_FLAT) || defined(CONFIG_BUILD_PROTECTED) \
+  || defined(__KERNEL__)

Review comment:
       
   
   
   
   > Actually the initial code don't have #ifdef/#endif, but @liguiding found that up_interrupt_context/enter_critical_section/leave_critical_section doesn't expose to userspace. That is why you see this guard in the mainline. Since the userspace should never call kmm_free in the interrupt context, it's reasonable to remove the delay free related code from userspace library until you report this issue.
   
   @xiaoxiang781216 Hmm, I understand. That's might be complicated than I though so I will not modify mm_malloc.c and mm_free.c this time.
   
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #843: Fix heap corruption

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #843:
URL: https://github.com/apache/incubator-nuttx/pull/843#issuecomment-617564116


   LGTM, thanks for fixing this tough corruption bug!


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] masayuki2009 commented on issue #843: Fix heap corruption

Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on issue #843:
URL: https://github.com/apache/incubator-nuttx/pull/843#issuecomment-617562855


   @xiaoxiang781216 I've just pushed with -f.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #843: Fix heap corruption

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #843:
URL: https://github.com/apache/incubator-nuttx/pull/843#discussion_r412630488



##########
File path: include/nuttx/mm/mm.h
##########
@@ -258,7 +259,8 @@ struct mm_heap_s
 
   struct mm_freenode_s mm_nodelist[MM_NNODES];
 
-#if defined(CONFIG_BUILD_FLAT) || defined(__KERNEL__)
+#if defined(CONFIG_BUILD_FLAT) || defined(CONFIG_BUILD_PROTECTED) \
+  || defined(__KERNEL__)

Review comment:
       From your report, it's very important to keep mm_heap_s layout consistent in all the memory model. So I think the more safe fix is that:
   1.Remove #ifdef/#endif around "struct mm_delaynode_s" and "struct mm_delaynode_s *mm_delaylist"
   2.Rremove #ifdef/#endif around nuttx/mm/mm_heap/mm_initialize.c:181




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] masayuki2009 commented on a change in pull request #843: Fix heap corruption

Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #843:
URL: https://github.com/apache/incubator-nuttx/pull/843#discussion_r412634476



##########
File path: include/nuttx/mm/mm.h
##########
@@ -258,7 +259,8 @@ struct mm_heap_s
 
   struct mm_freenode_s mm_nodelist[MM_NNODES];
 
-#if defined(CONFIG_BUILD_FLAT) || defined(__KERNEL__)
+#if defined(CONFIG_BUILD_FLAT) || defined(CONFIG_BUILD_PROTECTED) \
+  || defined(__KERNEL__)

Review comment:
       @xiaoxiang781216 Should we remove #ifdef/#endif in mmheap/mm_malloc.c and mmheap/mm_free.c as well?
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org