xiaoxiang781216 commented on a change in pull request #1369:
URL: https://github.com/apache/incubator-nuttx/pull/1369#discussion_r487427238
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > > Well the assembly code works. and it is not portable.
> > > I did not consider e.g. AddressSanitizer, Stack Smashing Protector. -
So you may be correct this may not work.
> >
> >
> > Then how do I handle this with your method? You need give a solution,
right?
> > > **Please Note: Fill the stack ONLY with the fill value as defined from
before or you will break all the monitors.**
> >
> >
> > I can't understand what you mean. I don't change the fill value(all
value same as before) in this patch. If I am wrong, please point the code
location I will correct them.
> > > Here is how I was looking at it. 1) I Draw a picture of the stack frame
> > > [ ]
> > > [ ]
> > > [ ]
> > > [ ]<---At the function
> > > [ marker1]
> > > [ ]
> > > [ what is here or not does not matter ]
> > > [ ]
> > > [ marker2 ]
> > > [ ] <- the allocation
> > > if the compiler messes with you and it looks like this, does not
matter.
> > > [ marker2]
> > > [ ]
> > > [ what is here or not does not matter ]
> > > [ ]
> > > [ marker1 ]
> > > [ ] <- the allocation
> > > safe = min of (&marker2, &marker1) word gets you the lowest point on
the stack you can write to,
> >
> >
> > Then do you think it's the right thing to memset "[ ] <- the
allocation"?!
> > The allocation region may contain:
> >
> > 1. The variables we defined between marker1 and marker2, but the
compiler free to move them to here
> > 2. The compiler may add more temporary variables as needed and put to
here
> > 3. ASAN or SSP may add the special mark to here too
> >
> > Please give a solution before I can use your method.
>
> I would make it a simple function and make it work. I would test it on
different archs,
>
As I said before, it work with the current compiler and option, doesn't mean
it work in the furture. So your propose isn't bettter than what PR done.
> If you are not sure the the compiler will do. Write it in assembly .
>
Yes, that's why I ask you two months ago, let me quote it here to remember
you:
> If you like go_nx_start approach, I can keep go_nx_start as before and let
up_use_stack skip to colorize the idle thread stack. But, I don't have time to
implement go_nx_start for each SoC.
> > > DO NOT Use MEMSET! (it will not write the correct pattern, and you
already know the call it a problem) Use a for() with the appropriate sized type
> >
> >
> > First, the original code use memset, not my patch. Second, why not use
memset? the orignial code work well.
>
> This is the original code
>
https://github.com/PX4/NuttX/blob/ec20f2e6c5cc35b2b9bbe942dea55eabb81297b6/arch/arm/src/stm32/stm32_start.c#L225-L256
>
> It does the correct thing.
>
> > "it will not write the correct pattern", could you explain the reason?
Sorry, I don't think this statement is right.
>
> The pattern is STACK_COLOR = 0xdeadbeef
>
> This is what is used to check penetration and it MUST match. Or you will
break the detection,.
I kindly point out memset serveral time, do you look at the realated code
before you make any negative statement? On the other arch, STACK_COLOR equals
0xaa!!! So please review my patch fully, please.
>
> > > Set the start address and write from bottom up or top down from or to
`safe`.
>
> Set the start address and write from bottom up to safe
> or
> write from top down from `safe` to the base.
>
> I feel we saying the same thing over and over again and not making
progress. Have you looked at the code generated and determined this will not
work or is that your opinion?
>
> If you post the generated code that proves you can not do it across the
permutations of ARCH, ASAN or SSP that will prove it does not work.
>
> You have to realize that there is also a risk with the #define approach,
that cause a problem when the call level get pushed deeper.
Yes, both approach aren't perfect, that's why I don't accept your propose.
Actually, I initially take your propose but find that many arch call memset to
colorize the stack which make your propose is impossible to complete.
----------------------------------------------------------------
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:
[email protected]
