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.

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

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

Conversation

GasInfinity
Copy link

While compiling with zig cc I encountered crashes due to undefined behaviour (It enables ubsan by default in debug/safe builds). This PR aims to fix some code which causes ub

Some of the fixes include:

  • Avoid passing NULL to src or dest in memcpy()
  • Avoid incrementing/decrementing a NULL pointer
  • Avoid incrementing a pointer by 0
  • Avoid doing signed bit shifting by casting to unsigned first

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.

@@ -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

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

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?

@skial skial mentioned this pull request Apr 29, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants