stb icon indicating copy to clipboard operation
stb copied to clipboard

hmget*_ts macros not completely thread safe

Open git-bruh opened this issue 4 years ago • 1 comments

Describe the bug hmget*_ts macros are not thread safe since they reassign the hash map pointer passed to them

To Reproduce Steps to reproduce the behavior:

Use stb_ds's hash map with thread sanitizer enabled, example code (pthread_cancel etc ommited):

#include <pthread.h>
#define STB_DS_IMPLEMENTATION
#include "stb_ds.h"

struct hm {
	int key;
	int value;
};

struct hm *h = NULL;

void *read(void *arg) {
	for (;;) {
		ptrdiff_t t = 0;
		hmget_ts(h, 1, t);
	}
}

int main() {
	hmput(h, 1, 2);
	pthread_create(&(pthread_t){0}, NULL, read, NULL);

	for (;;) {
		ptrdiff_t t = 0;
		hmget_ts(h, 1, t);
	}
}

TSAN output:

==================
WARNING: ThreadSanitizer: data race (pid=11042)
  Write of size 8 at 0x0000037c2200 by main thread:
    #0 main /home/testuser/git/ds.c:25:3 (a.out+0x4de64d)

  Previous read of size 8 at 0x0000037c2200 by thread T1:
    #0 read /home/testuser/git/ds.c:15:3 (a.out+0x4de3cc)

  Location is global 'h' of size 8 at 0x0000037c2200 (a.out+0x0000037c2200)

  Thread T1 (tid=11044, running) created by main thread at:
    #0 pthread_create <null> (a.out+0x48ee8a)
    #1 main /home/testuser/git/ds.c:21:2 (a.out+0x4de5f1)

SUMMARY: ThreadSanitizer: data race /home/testuser/git/ds.c:25:3 in main
==================

Expected behavior hmget*_ts macros should not reassign the original hash map pointer. Reassigning is only useful when the hashmap is NULL https://github.com/nothings/stb/blob/af1a5bc352164740c1cc1354942b1c6b72eacb8a/stb_ds.h#L1284

Can't think of a good fix other than the caller making a copy of the original pointer and passing that to stbds, like struct hm *tmp = hm; hmget_ts(tmp, 1, t); and ensuring that hm is not NULL to avoid a leak

The other fix is to make stbds expect a non-null hashmap but that will require changing the api a bit

git-bruh avatar Jan 04 '22 12:01 git-bruh

I don't think it requires changing the API. can just document that the get*_ts functions don't work on a NULL hash table. plus the hmgeti_ts would still work correctly.

nothings avatar Jan 04 '22 13:01 nothings