Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix multiple instances of undefined behaviour and crashes when using ubsan #677

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix multiple instances of undefined behaviour and crashes when using ubsan #677

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
06b360e
Fix multiple instances of undefined behaviour
Apr 18, 2024
74fe7a7
Cleanup unused variables that were used as an `offsetof`
Apr 18, 2024
96494e0
Merge branch 'HaxeFoundation:master' into fix-undefined-behaviour
GasInfinity Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/code.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ static int hl_read_i32( hl_reader *r ) {
b = r->b[r->pos++];
c = r->b[r->pos++];
d = r->b[r->pos++];
return a | (b<<8) | (c<<16) | (d<<24);
return (unsigned)a | ((unsigned)b<<8) | ((unsigned)c<<16) | ((unsigned)d<<24);
}

static int hl_read_index( hl_reader *r ) {
Expand Down
30 changes: 21 additions & 9 deletions src/gc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@
// 0x0000000YXXX0000
// 0x0007FY0YXXX0000
static int_val gc_hash( void *ptr ) {
int_val v = (int_val)ptr;
return (v ^ ((v >> 33) << 28)) & 0x0000000FFFFFFFFF;
uint_val v = (uint_val)ptr;
return (int_val)((v ^ ((v >> 33) << 28)) & 0x0000000FFFFFFFFF);
}
#endif

Expand Down Expand Up @@ -271,8 +271,12 @@ HL_PRIM void hl_add_root( void *r ) {
if( gc_roots_count == gc_roots_max ) {
int nroots = gc_roots_max ? (gc_roots_max << 1) : 16;
void ***roots = (void***)malloc(sizeof(void*)*nroots);
memcpy(roots,gc_roots,sizeof(void*)*gc_roots_count);
free(gc_roots);

if(gc_roots) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder: is this assumption ever violated? Can we use an assume intrinsic or similar?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, when hashlink starts gc_roots == NULL.

frame #5: 0x00007ffff7e0abc7 libhl.so.1`hl_add_root(r=0x00007ffff7f1c1a0) at gc.c:274:3
   271          if( gc_roots_count == gc_roots_max ) {
   272                  int nroots = gc_roots_max ? (gc_roots_max << 1) : 16;
   273                  void ***roots = (void***)malloc(sizeof(void*)*nroots);
-> 274                  memcpy(roots,gc_roots,sizeof(void*)*gc_roots_count);
   275                  free(gc_roots);
   276                  gc_roots = roots;
   277                  gc_roots_max = nroots;
(lldb) v
(void *) r = 0x00007ffff7f1c1a0
(int) nroots = 16
(void ***) roots = 0x0000000001194c60
(lldb) print gc_roots
(void ***) 0x0000000000000000

So, we can add that check or initialize it before calling hl_add_root() somewhere else.

memcpy(roots,gc_roots,sizeof(void*)*gc_roots_count);
free(gc_roots);
}

gc_roots = roots;
gc_roots_max = nroots;
}
Expand Down Expand Up @@ -322,7 +326,11 @@ HL_API void hl_register_thread( void *stack_top ) {

gc_global_lock(true);
hl_thread_info **all = (hl_thread_info**)malloc(sizeof(void*) * (gc_threads.count + 1));
memcpy(all,gc_threads.threads,sizeof(void*)*gc_threads.count);

if(gc_threads.threads) {
memcpy(all,gc_threads.threads,sizeof(void*)*gc_threads.count);
}

gc_threads.threads = all;
all[gc_threads.count++] = t;
gc_global_lock(false);
Expand Down Expand Up @@ -605,13 +613,17 @@ static hl_semaphore *mark_threads_done;
HL_PRIM void **hl_gc_mark_grow( gc_mstack *stack ) {
int nsize = stack->size ? (((stack->size * 3) >> 1) & ~1) : 256;
void **nstack = (void**)malloc(sizeof(void**) * nsize);
void **base_stack = stack->end - stack->size;
void **base_stack = stack->end ? stack->end - stack->size : NULL;
int avail = (int)(stack->cur - base_stack);
if( nstack == NULL ) {
out_of_memory("markstack");
return NULL;
}
memcpy(nstack, base_stack, avail * sizeof(void*));

if(base_stack) {
memcpy(nstack, base_stack, avail * sizeof(void*));
}

free(base_stack);
stack->size = nsize;
stack->end = nstack + nsize;
Expand Down Expand Up @@ -707,7 +719,7 @@ static int gc_flush_mark( gc_mstack *stack ) {
int pos = 0, nwords;
# ifdef GC_DEBUG
vdynamic *ptr = (vdynamic*)block;
ptr += 0; // prevent unreferenced warning
(void)ptr; // prevent unreferenced warning
# endif
if( !block ) {
__current_stack++;
Expand Down Expand Up @@ -749,7 +761,7 @@ static int gc_flush_mark( gc_mstack *stack ) {
# endif
while( pos < nwords ) {
void *p;
if( mark_bits && (mark_bits[pos >> 5] & (1 << (pos&31))) == 0 ) {
if( mark_bits && (mark_bits[pos >> 5] & (1u << (pos&31))) == 0 ) {
pos++;
block++;
continue;
Expand Down
1 change: 1 addition & 0 deletions src/hl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@
#endif

typedef intptr_t int_val;
typedef uintptr_t uint_val;
typedef long long int64;
typedef unsigned long long uint64;

Expand Down
57 changes: 36 additions & 21 deletions src/jit.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,10 +147,10 @@ typedef enum {
#define JNotZero JNeq

#define B(bv) *ctx->buf.b++ = (unsigned char)(bv)
#define W(wv) *ctx->buf.w++ = wv
#define W(wv) _jit_buf_w(ctx, wv)

#ifdef HL_64
# define W64(wv) *ctx->buf.w64++ = wv
# define W64(wv) _jit_buf_w64(ctx, wv)
#else
# define W64(wv) W(wv)
#endif
Expand Down Expand Up @@ -323,12 +323,22 @@ void error_i64() {
# endif
#endif

static inline void _jit_buf_w(jit_ctx* ctx, int wv) {
memcpy(ctx->buf.w++, &wv, sizeof(int));
}

#ifdef HL_64
static inline void _jit_buf_w64(jit_ctx* ctx, unsigned long long wv) {
memcpy(ctx->buf.w64++, &wv, sizeof(unsigned long long));
}
#endif

static void _jit_error( jit_ctx *ctx, const char *msg, int line );
static void on_jit_error( const char *msg, int_val line );

static preg *pmem( preg *r, CpuReg reg, int offset ) {
r->kind = RMEM;
r->id = 0 | (reg << 4) | (offset << 8);
r->id = 0 | ((unsigned)reg << 4) | ((unsigned)offset << 8);
return r;
}

Expand Down Expand Up @@ -846,7 +856,8 @@ static void patch_jump( jit_ctx *ctx, int p ) {
if( d < -128 || d >= 128 ) ASSERT(d);
*(char*)(ctx->startBuf + p) = (char)d;
} else {
*(int*)(ctx->startBuf + p) = BUF_POS() - (p + 4);
int patched = BUF_POS() - (p + 4);
memcpy(ctx->startBuf + p, &patched, sizeof(int));
}
}

Expand Down Expand Up @@ -2526,7 +2537,6 @@ static void jit_hl2c( jit_ctx *ctx ) {
// and pack and pass the args to callback_hl2c
preg p;
int jfloat1, jfloat2, jexit;
hl_type_fun *ft = NULL;
int size;
# ifdef HL_64
preg *cl = REG_AT(CALL_REGS[0]);
Expand Down Expand Up @@ -2558,7 +2568,7 @@ static void jit_hl2c( jit_ctx *ctx ) {
op64(ctx,MOV,cl,pmem(&p,Ebp,HL_WSIZE*2)); // load arg0
op64(ctx,MOV,tmp,pmem(&p,cl->id,0)); // ->t
op64(ctx,MOV,tmp,pmem(&p,tmp->id,HL_WSIZE)); // ->fun
op64(ctx,MOV,tmp,pmem(&p,tmp->id,(int)(int_val)&ft->ret)); // ->ret
op64(ctx,MOV,tmp,pmem(&p,tmp->id,(int)(int_val)offsetof(hl_type_fun, ret))); // ->ret
op32(ctx,MOV,tmp,pmem(&p,tmp->id,0)); // -> kind

op32(ctx,CMP,tmp,pconst(&p,HF64));
Expand Down Expand Up @@ -3405,7 +3415,11 @@ int hl_jit_function( jit_ctx *ctx, hl_module *m, hl_function *f ) {
preg *pc = REG_AT(CALL_REGS[0]);
vreg *sc = R(f->nregs); // scratch register that we temporary rebind
if( o->p3 >= 63 ) jit_error("assert");
memcpy(regids + 1, o->extra, o->p3 * sizeof(int));

if(o->extra) {
memcpy(regids + 1, o->extra, o->p3 * sizeof(int));
}

regids[0] = f->nregs;
sc->size = HL_WSIZE;
sc->t = &hlt_dyn;
Expand Down Expand Up @@ -4109,7 +4123,6 @@ int hl_jit_function( jit_ctx *ctx, hl_module *m, hl_function *f ) {
int size, jenter, jtrap;
int offset = 0;
int trap_size = (sizeof(hl_trap_ctx) + 15) & 0xFFF0;
hl_trap_ctx *t = NULL;
# ifndef HL_THREADS
if( tinf == NULL ) tinf = hl_get_thread(); // single thread
# endif
Expand All @@ -4125,14 +4138,14 @@ int hl_jit_function( jit_ctx *ctx, hl_module *m, hl_function *f ) {
if( !tinf ) {
call_native(ctx, hl_get_thread, 0);
op64(ctx,MOV,treg,PEAX);
offset = (int)(int_val)&tinf->trap_current;
offset = (int)(int_val)offsetof(hl_thread_info, trap_current);
} else {
offset = 0;
op64(ctx,MOV,treg,pconst64(&p,(int_val)&tinf->trap_current));
}
op64(ctx,MOV,trap,pmem(&p,treg->id,offset));
op64(ctx,SUB,PESP,pconst(&p,trap_size));
op64(ctx,MOV,pmem(&p,Esp,(int)(int_val)&t->prev),trap);
op64(ctx,MOV,pmem(&p,Esp,(int)(int_val)offsetof(hl_trap_ctx, prev)),trap);
op64(ctx,MOV,trap,PESP);
op64(ctx,MOV,pmem(&p,treg->id,offset),trap);

Expand Down Expand Up @@ -4166,7 +4179,7 @@ int hl_jit_function( jit_ctx *ctx, hl_module *m, hl_function *f ) {
} else {
op64(ctx,MOV,treg,pconst(&p,0));
}
op64(ctx,MOV,pmem(&p,Esp,(int)(int_val)&t->tcheck),treg);
op64(ctx,MOV,pmem(&p,Esp,(int)(int_val)offsetof(hl_trap_ctx, tcheck)),treg);

size = begin_native_call(ctx, 1);
set_native_arg(ctx,trap);
Expand All @@ -4176,7 +4189,7 @@ int hl_jit_function( jit_ctx *ctx, hl_module *m, hl_function *f ) {
op64(ctx,ADD,PESP,pconst(&p,trap_size));
if( !tinf ) {
call_native(ctx, hl_get_thread, 0);
op64(ctx,MOV,PEAX,pmem(&p, Eax, (int)(int_val)&tinf->exc_value));
op64(ctx,MOV,PEAX,pmem(&p, Eax, (int)(int_val)offsetof(hl_thread_info, exc_value)));
} else {
op64(ctx,MOV,PEAX,pconst64(&p,(int_val)&tinf->exc_value));
op64(ctx,MOV,PEAX,pmem(&p, Eax, 0));
Expand All @@ -4191,22 +4204,21 @@ int hl_jit_function( jit_ctx *ctx, hl_module *m, hl_function *f ) {
case OEndTrap:
{
int trap_size = (sizeof(hl_trap_ctx) + 15) & 0xFFF0;
hl_trap_ctx *tmp = NULL;
preg *addr,*r;
int offset;
if (!tinf) {
call_native(ctx, hl_get_thread, 0);
addr = PEAX;
RLOCK(addr);
offset = (int)(int_val)&tinf->trap_current;
offset = (int)(int_val)offsetof(hl_thread_info, trap_current);
} else {
offset = 0;
addr = alloc_reg(ctx, RCPU);
op64(ctx, MOV, addr, pconst64(&p, (int_val)&tinf->trap_current));
}
r = alloc_reg(ctx, RCPU);
op64(ctx, MOV, r, pmem(&p,addr->id,offset));
op64(ctx, MOV, r, pmem(&p,r->id,(int)(int_val)&tmp->prev));
op64(ctx, MOV, r, pmem(&p,r->id,(int)(int_val)offsetof(hl_trap_ctx, prev)));
op64(ctx, MOV, pmem(&p,addr->id, offset), r);
# ifdef HL_WIN
// erase eip (prevent false positive)
Expand Down Expand Up @@ -4385,7 +4397,8 @@ int hl_jit_function( jit_ctx *ctx, hl_module *m, hl_function *f ) {
{
jlist *j = ctx->jumps;
while( j ) {
*(int*)(ctx->startBuf + j->pos) = ctx->opsPos[j->target] - (j->pos + 4);
int patched = ctx->opsPos[j->target] - (j->pos + 4);
memcpy(ctx->startBuf + j->pos, &patched, sizeof(int));
j = j->next;
}
ctx->jumps = NULL;
Expand Down Expand Up @@ -4485,23 +4498,25 @@ void *hl_jit_code( jit_ctx *ctx, hl_module *m, int *codesize, hl_debug_infos **d
fabs = (unsigned char*)code + (int)(int_val)fabs;
}
}
if( (code[c->pos]&~3) == (IS_64?0x48:0xB8) || code[c->pos] == 0x68 ) // MOV : absolute | PUSH
*(void**)(code + c->pos + (IS_64?2:1)) = fabs;
else {
if( (code[c->pos]&~3) == (IS_64?0x48:0xB8) || code[c->pos] == 0x68 ) { // MOV : absolute | PUSH
memcpy(code + c->pos + (IS_64?2:1), &fabs, sizeof(void*));
} else {
int_val delta = (int_val)fabs - (int_val)code - (c->pos + 5);
int rpos = (int)delta;
if( (int_val)rpos != delta ) {
printf("Target code too far too rebase\n");
return NULL;
}
*(int*)(code + c->pos + 1) = rpos;

memcpy(code + c->pos + 1, &rpos, sizeof(int));
}
c = c->next;
}
// patch switchs
c = ctx->switchs;
while( c ) {
*(void**)(code + c->pos) = code + c->pos + (IS_64 ? 14 : 6);
void* patched = code + c->pos + (IS_64 ? 14 : 6);
memcpy(code + c->pos, &patched, sizeof(void*));
c = c->next;
}
// patch closures
Expand Down
6 changes: 5 additions & 1 deletion src/module.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -622,7 +622,11 @@ static void hl_module_init_constant( hl_module *m, hl_constant *c ) {
static void hl_module_add( hl_module *m ) {
hl_module **old_modules = cur_modules;
hl_module **new_modules = (hl_module**)malloc(sizeof(void*)*(modules_count + 1));
memcpy(new_modules, old_modules, sizeof(void*)*modules_count);

if(old_modules) {
memcpy(new_modules, old_modules, sizeof(void*)*modules_count);
}

new_modules[modules_count] = m;
cur_modules = new_modules;
modules_count++;
Expand Down
8 changes: 6 additions & 2 deletions src/std/bytes.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ HL_PRIM vbyte *hl_copy_bytes( const vbyte *ptr, int size ) {
}

HL_PRIM void hl_bytes_blit( char *dst, int dpos, char *src, int spos, int len ) {
memmove(dst + dpos,src+spos,len);
if(dst && src) {
memmove(dst + dpos,src + spos,len);
}
}

HL_PRIM int hl_bytes_compare( vbyte *a, int apos, vbyte *b, int bpos, int len ) {
Expand Down Expand Up @@ -143,7 +145,9 @@ HL_PRIM int hl_bytes_rfind( vbyte *where, int len, vbyte *which, int wlen ) {
}

HL_PRIM void hl_bytes_fill( vbyte *bytes, int pos, int len, int value ) {
memset(bytes+pos,value,len);
if(bytes) {
Copy link
Member

@Aurel300 Aurel300 Apr 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this will now fail silently? If this check is performed, might as well emit a warning or something? Additionally, I guess this situation (bytes == NULL) is also meant to be unreachable with how the bytes wrapper works Haxe-side?

Copy link
Author

@GasInfinity GasInfinity Apr 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That happened when I was testing the changes with some openfl templates. It seems that somewhere it passes NULL to hl_bytes_fill and hl_bytes_blit.

Edit: About the warning, where should it be emitted? to stderr?

memset(bytes+pos,value,len);
}
}


Expand Down
2 changes: 1 addition & 1 deletion src/std/debug.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,7 +349,7 @@ HL_API void *hl_debug_read_register( int pid, int thread, int reg, bool is64 ) {
// peek FP ptr
char *addr = (char*)ptrace(PTRACE_PEEKUSER,thread,get_reg(-1),0);
void *out = NULL;
hl_debug_read(pid, addr + (-((int_val)r)-1), (vbyte*)&out, sizeof(void*));
hl_debug_read(pid, (unsigned char*)(addr + (-((int_val)r)-1)), (vbyte*)&out, sizeof(void*));
return out;
}
return (void*)ptrace(PTRACE_PEEKUSER,thread,r,0);
Expand Down
6 changes: 5 additions & 1 deletion src/std/maps.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,11 @@ typedef struct {

static void hl_freelist_resize( hl_free_list *f, int newsize ) {
hl_free_bucket *buckets = (hl_free_bucket*)hl_gc_alloc_noptr(sizeof(hl_free_bucket)*newsize);
memcpy(buckets,f->buckets,f->head * sizeof(hl_free_bucket));

if(f->buckets) {
memcpy(buckets,f->buckets,f->head * sizeof(hl_free_bucket));
}

f->buckets = buckets;
f->nbuckets = newsize;
}
Expand Down
Loading