Skip to content

Fix parallel access crashes and misbehavior - #136

Merged
dra27 merged 9 commits into
ocaml:masterfrom
jmid:single-global-lock-par-fix
Jun 26, 2024
Merged

Fix parallel access crashes and misbehavior#136
dra27 merged 9 commits into
ocaml:masterfrom
jmid:single-global-lock-par-fix

Conversation

@jmid

@jmidjmid commented Apr 16, 2024

Copy link
Copy Markdown
Member

Parallel usage is memory unsafe (read: may crash) as documented in #120, ocaml/ocaml#11607, and ocaml/ocaml#13046.

This PR goes for the simplest possible fix: adding a single global lock by dusting off the first commit of https://github.com/dra27/flexdll/tree/sledgehammer and suitable rebasing, renaming, and error handling.
Author credit thus goes to @dra27 - any errors are mine.

For the error handling, I've tried to make it fit with @shym's TLS-based error handling from #112.
I'm unsure how to test these error code paths though without explicitly mocking with the source code to create an invalid lock handle.

With the fix

(these have been tested under MinGW in a Cygwin-shell)

Comment threadflexdll.c
Comment threadflexdll.c
Comment threadflexdll.c Outdated
Comment threadflexdll.c Outdated
goto again;
}
} else {
if (WaitForSingleObject(units_mutex, INFINITE) == WAIT_FAILED) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (WaitForSingleObject(units_mutex, INFINITE) ==WAIT_FAILED) {
if (WaitForSingleObject(units_mutex, INFINITE) !=WAIT_OBJECT_0) {

This would cover the improbable (impossible?) case of the mutex having been abandoned.
https://learn.microsoft.com/en-us/windows/win32/sync/using-mutex-objects

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I ended up reverting this one.

WAIT_ABANDONED does not offer GetLastError-integration, so I started adding an explicit error message case for it - only to then hit it during testing.

I therefore suppose these two cases were written to handle both WAIT_OBJECT_0 and WAIT_ABANDONED as success-cases (WAIT_TIMEOUT should not happen with INFINITE...)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My reasoning I think is that WAIT_ABANDONED should be treated like success?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(i.e. I agree with @jmid's assessment!)

Comment threadflexdll.c
err = get_tls_error(TLS_ERROR_NOP);
if(err == NULL) return NULL;

if (WaitForSingleObject(units_mutex, INFINITE) == WAIT_FAILED) {

@MisterDAMisterDAApr 17, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (WaitForSingleObject(units_mutex, INFINITE) ==WAIT_FAILED) {
if (WaitForSingleObject(units_mutex, INFINITE) !=WAIT_OBJECT_0) {

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

(same for this one)

Comment threadflexdll.c Outdated
Comment threadflexdll.c Outdated
@jmid
jmidforce-pushed the single-global-lock-par-fix branch from 217db5d to 67efa7dCompareApril 18, 2024 17:06

@MisterDAMisterDA left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I've tested an finally understood why the code is correct, thanks Jan! I think this is good to go.
Minor suggestion still.

Comment threadflexdll.c Outdated
@jmid
jmidforce-pushed the single-global-lock-par-fix branch from 47480bc to 4950924CompareMay 16, 2024 12:35
@jmid

jmid commented May 16, 2024

Copy link
Copy Markdown
MemberAuthor

I've addressed the last minor comment, added a CHANGES entry, and rebased on master.

@dra27
dra27 merged commit 5719f5a into ocaml:masterJun 26, 2024
@dra27

Copy link
Copy Markdown
Member

Thank you both for your work on this, and sorry for taking quite so long to merge it!

@jmid
jmid deleted the single-global-lock-par-fix branch June 26, 2024 08:53
Sign up for freeto 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.

3 participants

@jmid@dra27@MisterDA