build.yml: added sanitized build with UndefinedBehaviorSanitizer

[firewave]

No description provided.

[firewave]

This could be combined with the existing ASAN build.

[firewave]

Example of one of the alignment errors (no idea if it is valid):

libipm_send.c:234:9: runtime error: store to misaligned address 0x55d733cc2c0d for type 'unsigned int', which requires 4 byte alignment
0x55d733cc2c0d: note: pointer points here
 65 73 74 75 6c 69 62  69 70 6d 5f 72 65 63 76  5f 63 61 6c 6c 73 2e 63  00 00 02 f3 6c 73 2e 63  00
             ^ 
    #0 0x7f5d4a22e49c in append_int32_type /home/runner/work/xrdp/xrdp/libipm/libipm_send.c:234:9
    #1 0x7f5d4a22e49c in libipm_msg_out_appendv /home/runner/work/xrdp/xrdp/libipm/libipm_send.c:437:26
    #2 0x7f5d4a22f926 in libipm_msg_out_simple_send /home/runner/work/xrdp/xrdp/libipm/libipm_send.c:591:14
    #3 0x55d72f07d5c1 in test_libipm_receive_bad_header_fn /home/runner/work/xrdp/xrdp/tests/libipm/test_libipm_recv_calls.c:803:14
    #4 0x55d72f0821a0 in srunner_run_tagged (/home/runner/work/xrdp/xrdp/tests/libipm/.libs/test_libipm+0x401a0) (BuildId: efd0608ac418cd67594a33f3171455bb68dfe8c5)
    #5 0x55d72f077cd9 in main /home/runner/work/xrdp/xrdp/tests/libipm/test_libipm_main.c:261:5
    #6 0x7f5d49e2a1c9  (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
    #7 0x7f5d49e2a28a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
    #8 0x55d72f04c9a4 in _start (/home/runner/work/xrdp/xrdp/tests/libipm/.libs/test_libipm+0xa9a4) (BuildId: efd0608ac418cd67594a33f3171455bb68dfe8c5)

SUMMARY: UndefinedBehaviorSanitizer: misaligned-pointer-use libipm_send.c:234:9 

[firewave]

Example of one of the alignment errors (no idea if it is valid):

-Wcast-align might show these issues at compile-time.

[matt335672]

The specific warning you point out above relates to the use of the out_uint32_le() macro which is defined as follows:-

#if defined(B_ENDIAN) || defined(NEED_ALIGN)
#define out_uint32_le(s, v) do \
    { \
        S_CHECK_REM_OUT((s), 4); \
        *((s)->p) = (unsigned char)((v) >> 0); \
        (s)->p++; \
        *((s)->p) = (unsigned char)((v) >> 8); \
        (s)->p++; \
        *((s)->p) = (unsigned char)((v) >> 16); \
        (s)->p++; \
        *((s)->p) = (unsigned char)((v) >> 24); \
        (s)->p++; \
    } while (0)
#else
#define out_uint32_le(s, v) do \
    { \
        S_CHECK_REM_OUT((s), 4); \
        *((unsigned int*)((s)->p)) = (v); \
        (s)->p += 4; \
    } while (0)
#endif

On x86_64, NEED_ALIGN is false, and so a single MOV instruction is emitted to write the 32-bit word to memory regardless of alignment. This will trigger the sanitizer.

The macro is used fairly extensively:-

$ find . -name \*.c -exec grep out_uint32_le {} + | wc -l
812

There are two qualifications to this approach:

1. Using microcode to 'paper over' unaligned accesses will clearly have some sort of performance hit. We haven't quantified this, and there may be no point, as different processors may perform in different ways.
2. Modern compilers optimise the generated code in astonishing ways. While I was researching this comment I wrote this code:-

#include <stdint.h>
#include <stdio.h>

int main()
{
    const uint32_t val = 0x58524450;
    char *p;
    union
    {
        unsigned char buff[100];
        unsigned int i;
    } target;

    // Write a 32-bit word with MOV
    *(uint32_t*)&target.buff[1] = val;

    // Write a 32-bit word as bytes
    p = &target.buff[5];
    *p++ = (unsigned char)(val >> 0);
    *p++ = (unsigned char)(val >> 8);
    *p++ = (unsigned char)(val >> 16);
    *p++ = (unsigned char)(val >> 24);

    int i;
    for (i = 1 ; i < 9 ; ++i)
    {
        printf("[%d] = %02x\n", i, target.buff[i]);
    }
}

I built it with gcc 14.2.0 using cc -O2 -g -o temp temp.c. Generated code from gdb looks like this:-

(gdb) disassemble/m main
Dump of assembler code for function main:
5	{
   0x0000000000001080 <+0>:	endbr64
   0x0000000000001084 <+4>:	push   %r12
   0x0000000000001086 <+6>:	push   %rbp
   0x0000000000001087 <+7>:	lea    0xf76(%rip),%rbp        # 0x2004
   0x000000000000108e <+14>:	push   %rbx
   0x0000000000001094 <+20>:	sub    $0x70,%rsp

6	    const uint32_t val = 0x58524450;
7	    char *p;
8	    union
9	    {
10	        unsigned char buff[100];
11	        unsigned int i;
12	    } target;
13	
14	    // Write a 32-bit word with MOV
15	    *(uint32_t*)&target.buff[1] = val;
   0x000000000000108f <+15>:	mov    $0x1,%ebx
   0x0000000000001098 <+24>:	mov    %fs:0x28,%rax
   0x00000000000010a1 <+33>:	mov    %rax,0x68(%rsp)
   0x00000000000010a6 <+38>:	movabs $0x5852445058524450,%rax
   0x00000000000010b0 <+48>:	mov    %rsp,%r12
   0x00000000000010b3 <+51>:	mov    %rax,0x1(%rsp)

16	
17	    // Write a 32-bit word as bytes
18	    p = &target.buff[5];
19	    *p++ = (unsigned char)(val >> 0);
20	    *p++ = (unsigned char)(val >> 8);
21	    *p++ = (unsigned char)(val >> 16);
22	    *p++ = (unsigned char)(val >> 24);
23	
24	    int i;

25	    for (i = 1 ; i < 9 ; ++i)
   0x00000000000010b8 <+56>:	nopl   0x0(%rax,%rax,1)
   0x00000000000010d1 <+81>:	add    $0x1,%rbx
   0x00000000000010da <+90>:	cmp    $0x9,%rbx
   0x00000000000010de <+94>:	jne    0x10c0 <main+64>

26	    {
27	        printf("[%d] = %02x\n", i, target.buff[i]);
28	    }
29	}
   0x00000000000010e0 <+96>:	mov    0x68(%rsp),%rax
   0x00000000000010e5 <+101>:	sub    %fs:0x28,%rax
   0x00000000000010ee <+110>:	jne    0x10fb <main+123>
   0x00000000000010f0 <+112>:	add    $0x70,%rsp
   0x00000000000010f4 <+116>:	xor    %eax,%eax
   0x00000000000010f6 <+118>:	pop    %rbx
   0x00000000000010f7 <+119>:	pop    %rbp
   0x00000000000010f8 <+120>:	pop    %r12
   0x00000000000010fa <+122>:	ret
   0x00000000000010fb <+123>:	call   0x1060 <__stack_chk_fail@plt>

End of assembler dump.

Although I've specified two separate code segments to write the target word as an unaligned access and separately as bytes, the compiler has emitted a single unaligned mov instruction for all 8 bytes!

0x00000000000010a6 <+38>:	movabs $0x5852445058524450,%rax

What are your thoughts on this? We may need some wider discussions.

[firewave]

What are your thoughts on this? We may need some wider discussions.

I have no thoughts on that. I rarely (never?) had to deal with alignment stuff so it goes way over my head. It should probably done outside of this PR and the current state should be merged.

Thanks for the great analysis. I will give it a proper read later and possibly learn something from it.

Also as mentioned it might be worth looking at the -Wcast-align warnings.