Re: [Valgrind-developers] Fix executable stacks for assembly files

[Florian K. <fl...@ei...>]

On 27.09.2015 21:19, Matthias Schwarzott wrote:
> Hi there!
> 
> This patch will fix all assembler files that do not declare their stack
> as non-executable.
> 
> All assembler files that do not contain this code tells the linker to
> mark the stack executable:
> 
>   .section .note.GNU-stack,"",@progbits
> OR
>   .section .note.GNU-stack,"",%progbits
> 
> My patch creates a macro that contains either version or nothing if the
> platform does not require it.
> 
> Then all asm-files are changed to unconditionally call this macro at the
> end of file.
> 

It's nice to clean this up. Thanks!

> Additionally it removes an unneeded inclusion of "config.h" from
> "dispatch-ppc32-linux.S".

No.  dispatch-ppc32-linux.S has an #ifdef HAS_ALTIVEC and that symbol is
being defined in config.h .


> 
> The only thing I am not sure about is: On which platforms to use
> @progbits or %progbits.

m_trampoline.S has this:

#if defined(VGO_linux)
/* Let the linker know we don't need an executable stack */
#  if defined(VGP_arm_linux)
   .section .note.GNU-stack,"",%progbits
#  else
   .section .note.GNU-stack,"",@progbits
#  endif
#endif

That code is shared for all platforms. A web search provides further
evidence that on ARM the @ character introduces a comment in assembler
code. On other platforms, the choice of @progbits or %progbits
apparently does not matter. At least on mips a grep shows:

./coregrind/m_syswrap/syscall-mips64-linux.S:.section
.note.GNU-stack,"",%progbits
./coregrind/m_dispatch/dispatch-mips64-linux.S:.section
.note.GNU-stack,"",@progbits

So let's run with the above ifdeffery.

What I did not quite understand why (sometimes) you moved code outside
the #ifdef that guards the complete contents of a file. For instance:

--- a/coregrind/m_syswrap/syscall-x86-solaris.S
+++ b/coregrind/m_syswrap/syscall-x86-solaris.S
@@ -28,9 +28,10 @@
   The GNU General Public License is contained in the file COPYING.
 */

+#include "pub_core_basics_asm.h"
+
 #if defined(VGP_x86_solaris)

-#include "pub_core_basics_asm.h"
 #include "pub_core_vkiscnums_asm.h"
 #include "libvex_guest_offsets.h"

@@ -270,6 +271,9 @@ ML_(blksys_finished_DRET):	.long 4b

 #endif // defined(VGP_x86_solaris)

+/* Let the linker know we don't need an executable stack */
+MARK_STACK_NO_EXEC
+

I don't see a need for that or did you encounter some sort of problem?

   Florian