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

Pi-hole FTL v4.2.3 #519

Merged
merged 9 commits into from Feb 26, 2019
Merged

Pi-hole FTL v4.2.3 #519

merged 9 commits into from Feb 26, 2019

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Feb 22, 2019

This release fixes #496

Issue

DNS requests and responses can happen over UDP or TCP. UDP is the most common, but TCP is used often for large responses or by some specific products (we have seen this with Netflix in the past). Handling TCP connections requires more communication with the client and server, so pihole-FTL creates TCP helper processes by forking.

When dynamically allocated shared memory objects get resized, they get resized, and then re-mapped. The problem is that if our shared memory object is now significantly larger and if this happens on a TCP handler process, the other processes were not notified about the virtual memory change keep using the old shared memory, i.e., they were using dangling pointers. This caused a memory corruption or access error when accessed.

Solution

This pull requests ensures that, when the space of virtual addresses changes, the other forks get notified and can re-map the shared memory objects to get possibly updated virtual memory addresses for their pointers.

AzureMarker and others added 9 commits February 21, 2019 17:07
Signed-off-by: Mcat12 <newtoncat12@yahoo.com>
Signed-off-by: DL6ER <dl6er@dl6er.de>

Conflicts:
	shmem.c
…t that at least one of the objects has changed

Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
…ory objects into all running processes, even if they are forks

Signed-off-by: DL6ER <dl6er@dl6er.de>
…a new virtual address

Signed-off-by: DL6ER <dl6er@dl6er.de>
This was causing an issue because each process had a different value
for the next string position, and caused corruption in the string
shared memory.

Signed-off-by: Mcat12 <newtoncat12@yahoo.com>
The version on development is 3, so this version should be 4.

Signed-off-by: Mcat12 <newtoncat12@yahoo.com>
@DL6ER DL6ER added this to the v4.2.3 milestone Feb 22, 2019
close(fd);
// needed after having called ftruncate()
if(resize)
close(fd);
Copy link
Contributor

Choose a reason for hiding this comment

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

The file descriptor could be closed at the end of the first if block where we resize the memory, instead of waiting until here to close it. It's only used in a logging statement outside of the if block (and doesn't need to be there).

Copy link
Contributor

@AzureMarker AzureMarker left a comment

Choose a reason for hiding this comment

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

No changes required, so approving in case we don't push any more commits

@DL6ER DL6ER marked this pull request as ready for review February 22, 2019 17:33
@dschaper dschaper merged commit aae487e into master Feb 26, 2019
@DL6ER DL6ER mentioned this pull request Feb 26, 2019
5 tasks
@AzureMarker AzureMarker deleted the hotfix/v4.2.3 branch May 18, 2019 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FTL Crash
6 participants