Skip to content

Comments

Fix fclose sync#2

Merged
WalterBright merged 2 commits intoDigitalMars:masterfrom
CyberShadow:fclose-sync
May 8, 2016
Merged

Fix fclose sync#2
WalterBright merged 2 commits intoDigitalMars:masterfrom
CyberShadow:fclose-sync

Conversation

@CyberShadow
Copy link
Member

Fixes D issue 13727

https://issues.dlang.org/show_bug.cgi?id=13727


The root of the problem is that UnlockSemaphoreNested is messing with shared resources after releasing the lock to them. lock dec _iSemLockCtrs[edx * 2] is the point at which the lock is effectively released, however the macro proceeds to call _DestroySemaphore after releasing said lock. This ends up badly when another thread simultaneously acquires this semaphore (for another file) and attempts to use it.

The solution I went for is to simply disable deleting semaphores on fclose (or, well, when __fpunlock detects an fclose). The side effect, in its worst case (opening and then closing _NFILE files), is a temporary leak of up to _NFILE semaphores until the program exits.

I'd like to add that the synchronization code generally raises red flags from a number of other points:

  • Using semaphores, which are a global (machine-scoped) synchronization primitive
  • "Resolving" the global aspect above by using the current time in milliseconds as a namespace (GetTickCount doesn't actually even have millisecond precision)
  • Using named objects, which causes every semaphore operation to go through some synchronized hash table managed by the operating system
  • _EnsureSemaphore, called every time from _WaitSemaphore and _ReleaseSemaphore, performs an unconditional itoa and a questionable unsynchronized loop

I think ideally it should be replaced by plain CRITICAL_SECTION objects (which, AFAIU, function much like the good parts of the current implementation - spin locks + notification-based synchronization), but I guess overly invasive changes are best avoided here.

Fixes strange fclose() return values
The semaphore might be immediately reused by a file newly opened
in another thread, thus creating a race condition.
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