davids5 commented on a change in pull request #1369:
URL: https://github.com/apache/incubator-nuttx/pull/1369#discussion_r487423252
##########
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,
If you are not sure the the compiler will do. Write it in assembly .
>
> > 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,.
> > 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.
----------------------------------------------------------------
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]
