Skip to content

Fix for bug CORE-5515 #314

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
Merged
merged 3 commits into from
Apr 20, 2021
Merged

Fix for bug CORE-5515 #314

merged 3 commits into from
Apr 20, 2021

Conversation

hvlad
Copy link
Member

@hvlad hvlad commented Mar 14, 2021

Fixed bug CORE-5515

hvlad added 3 commits March 14, 2021 01:56
@hvlad
Enforce equal binary layout of event_t structure for both 32 and 64-b…
ffaf62b 
…it platforms as its instances is used in shared memory.

It fixed bug CORE-5515 : When 32bit and 64bit FB 3 servers run on one Windows machine concurrently, Firebird services freeze several minutes after first disconnect.
@hvlad
Use correct type for event handle and remove few unneeded casts.
45bdd2e
@hvlad
Added check for correct version of shared memory used in MappingIpc.
60ea1d2
@hvlad hvlad self-assigned this Mar 14, 2021
dyemanov
dyemanov reviewed Mar 14, 2021
View reviewed changes
src/jrd/Mapping.cpp
if (sMem->mhb_type != SharedMemoryBase::SRAM_MAPPING_RESET ||
sMem->mhb_header_version != MemoryHeader::HEADER_VERSION ||
sMem->mhb_version != MAPPING_VERSION)
{
Copy link
Member

Choose a reason for hiding this comment

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

Is it safe to access mhb_* members without a mutex acquired? What if shmem is being initialized by another process right at the moment?

Copy link
Member Author

Choose a reason for hiding this comment

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

shmem is not accesible by other processes while it is initialized.
Also, note, on non-Windows - shmem contains mutex that should be acquired - should we access it when shmem binary layout is not known\verified ?
And, finally - former fb_assert() does the same - check before muex is acquired ;)

@hvlad
Copy link
Member Author

hvlad commented Mar 14, 2021

@hvlad for master, can't just use alignas?

Yes, we can. But I consider old-style union way is more clear\explicit at this place.
With union it is explicitly clear that last member occupy 8 bytes, while with alignas it is far not that obvious.

@hvlad hvlad merged commit 90270cd into master Apr 20, 2021
@hvlad hvlad linked an issue May 1, 2021 that may be closed by this pull request
When 32bit and 64bit FB 3 servers run on one Windows machine concurrently, Firebird services freeze several minutes after first disconnect. [CORE5515] #5784
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dyemanov dyemanov dyemanov left review comments

Assignees

@hvlad hvlad

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

When 32bit and 64bit FB 3 servers run on one Windows machine concurrently, Firebird services freeze several minutes after first disconnect. [CORE5515]
3 participants
@hvlad @asfernandes @dyemanov