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

Remove socket deletion from epoll #3432

Merged
merged 1 commit into from Jun 17, 2019
Merged

Remove socket deletion from epoll #3432

merged 1 commit into from Jun 17, 2019

Conversation

albertomn86
Copy link
Contributor

@albertomn86 albertomn86 commented May 30, 2019

Description

The file descriptor deletion from the epoll has been removed from the HandleSecure() function.

wazuh/src/remoted/secure.c

Lines 198 to 200 in 090986e

if (wnotify_delete(notify, sock_client) < 0) {
merror("wnotify_delete(%d): %s (%d)", sock_client, strerror(errno), errno);
}

This deletion is not necessary because the file descriptor is removed from it after all the file descriptors referring to the socket have been closed. It is described here:

Will closing a file descriptor cause it to be removed from all epoll interest lists?

Yes, but be aware of the following point. A file descriptor is a reference to an open file description (see open(2)). Whenever a file descriptor is duplicated via dup(2), dup2(2), fcntl(2) F_DUPFD, or fork(2), a new file descriptor referring to the same open file description is created. An open file description con‐ tinues to exist until all file descriptors referring to it have been closed.

Calling close() on a file descriptor will remove any kevents that reference the descriptor.

In high load environments, this deletion might cause a race condition and the following error could appear:

2019/05/20 05:33:12 ossec-remoted: ERROR: wnotify_delete(130): Bad file descriptor (9)

Tests

  • Compilation without warnings in every supported platform
    • Linux
  • Source installation
  • Source upgrade
  • Memory tests
    • Valgrind report for affected components
    • CPU impact
    • RAM usage impact
  • Retrocompatibility with older Wazuh versions
  • Working on cluster environments

@albertomn86 albertomn86 requested a review from vikman90 May 30, 2019 15:25
@vikman90 vikman90 requested a review from snaow May 30, 2019 15:28
@vikman90 vikman90 self-assigned this May 30, 2019
@vikman90 vikman90 modified the milestones: 22nd week, 23rd week May 31, 2019
Copy link
Member

@vikman90 vikman90 left a comment

Choose a reason for hiding this comment

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

LGTM!

@vikman90 vikman90 added this to Under Review in Wazuh 3.10.0 via automation Jun 3, 2019
@vikman90 vikman90 added type/bug Something isn't working module/remote labels Jun 3, 2019
@vikman90 vikman90 changed the base branch from 3.9 to 3.10 June 3, 2019 11:43
@albertomn86
Copy link
Contributor Author

Valgrind report

 ==27534== Memcheck, a memory error detector
==27534== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==27534== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==27534== Command: /var/ossec/bin/ossec-remoted -fdd
==27534== Parent PID: 3948
==27534==
==27534==
==27534== HEAP SUMMARY:
==27534==     in use at exit: 55 bytes in 6 blocks
==27534==   total heap usage: 12,016 allocs, 12,010 frees, 5,171,311 bytes allocated
==27534==
==27534== LEAK SUMMARY:
==27534==    definitely lost: 0 bytes in 0 blocks
==27534==    indirectly lost: 0 bytes in 0 blocks
==27534==      possibly lost: 0 bytes in 0 blocks
==27534==    still reachable: 55 bytes in 6 blocks
==27534==         suppressed: 0 bytes in 0 blocks
==27534== Reachable blocks (those to which a pointer was found) are not shown.
==27534== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==27534==
==27534== For counts of detected and suppressed errors, rerun with: -v
==27534== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
==27534== could not unlink /tmp/vgdb-pipe-from-vgdb-to-27534-by-root-on-???
==27534== could not unlink /tmp/vgdb-pipe-to-vgdb-from-27534-by-root-on-???
==27534== could not unlink /tmp/vgdb-pipe-shared-mem-vgdb-27534-by-root-on-???

@vikman90 vikman90 modified the milestones: 23rd week, 24th week Jun 10, 2019
Wazuh 3.10.0 automation moved this from Under Review to Reviewer approved Jun 14, 2019
@vikman90 vikman90 merged commit b82ae82 into 3.10 Jun 17, 2019
Wazuh 3.10.0 automation moved this from Reviewer approved to Done Jun 17, 2019
@vikman90 vikman90 deleted the remove-wnotify-delete branch June 25, 2019 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module/remote type/bug Something isn't working
Projects
No open projects
Wazuh 3.10.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants