Skip to content

Commit

Permalink
Added support to lock files on Windows and major rewrite to reintrodu…
Browse files Browse the repository at this point in the history
…ce reference counting and remove unused code.

- In Windows build, replaced usage of fcntl with cmd F_SETLKW with
  Win32 APIs to do file locking (LockFileEx & UnlockFileEx).
- Reintroduced the reference counting initially present in the class
  which is necessary to correctly handle merging of rules. This allows
  for correctly closing the file and removing the associated entry from
  m_handlers when the file is no longer used.
  - The need for reference counting can be seen in the example
  simple_example_using_c, where rules are initially loaded locally and
  then further rules are loaded remotely. This will initially open a
  shared file for a log, then in order to merge rules, the shared file
  is opened again for the new configuration. Then, the previous
  configuration closes the shared file on destruction. That is, two
  consecutive opens are done on a shared file, which is followed by
  a close. If the shared file is not reference counted, the shared file
  will be closed while there is still a reference active. The current
  version works because closing of the file has been disabled after
  reference counting was removed.
- Replaced `std::vector` data structure with `std::unordered_map` to
  improve lookup/update times, and simplify code.
- Removed unused code
  - Shared memory to store msc_file_handler structure
    - Initially SharedFiles used shared memory to store information
    about each shared file, including its file pointer and a mutex to
    synchronize access to the file on write. See code at commit 01c13da,
    in particular, usage of lock & fp fields in the msc_file_handler_t
    structure.
    - At that time, msc_file_handler_t included reference counting too
    with the using_it field, which was incremented when a file was
    opened and decremented on close. If the reference count reached
    zero, the shared file would be closed, the lock destroyed and the
    file handler entry removed from m_handlers.
    - Reference counting was removed in commit 7f9cd76, which
    introduced the following issues in SharedFiles::close:
      - No longer closes the file pointer.
        - The file pointer appears to be reset when a.second = 0, but
	this is a local copy of the data pair obtained from m_handlers,
	so this is essentially a nop (updating a local variable that is
	not referenced later in the function).
        - NOTE: The file pointer was moved out of the shared memory in
	this commit too, and stored alongside the msc_file_handler_t
	instance in the m_handlers entry associated to the shared file.
      - The lock is no longer destroyed.
      - The shared memory is marked to be destroyed in the call to:
      shmctl(a.first->shm_id_structure, IPC_RMID, NULL);
      - The shared file entry is not removed from m_handlers, so:
        - the file pointer is still valid, which is how writing to the
	file continues to work,
        - the reference to the shared memory is also present and will
	be marked to be destroyed whenever close is called again on the
	shared file.
    - File locking using the mutex in msc_file_handler_t was replaced in
    commit 3d20304 with usage of fcntl with cmd F_SETLKW.
    - At this time, it appears that the shared memory is no longer used,
    as the file pointer and locking no longer depend on it.
  - MODSEC_USE_GENERAL_LOCK
    - This code is introduced commit 7f9cd76 and is enabled if
    MODSEC_USE_GENERAL_LOCK` is defined.
    - The define is commented out in the source code since the original
    commit and is not present in the build configuration either.
    - In commit ff9152e, in the SharedFiles constructor, the
    initialization of the local variable toBeCreated is removed. This
    means that in this version, if MODSEC_USE_GENERAL_LOCK is enabled,
    execution of the code that checks on toBeCreated is undefined.
    - Then, in commit 9b40a04, the variable toBeCreated is initialized
    again, but is now set to false, which means that if
    MODSEC_USE_GENERAL_LOCK is enabled, the shared memory and lock it
    uses will *not* be initialized and thus doesn't work (execution of
    the current version will result in trying to acquire a lock that
    will be null).
    - I conclude that the feature is not used and can be removed.
      - Additionally, if it were working, I think the lock should be
      used in SharedFiles::write as well, which is a reader of the
      underlying data structures protected by this lock when they're
      modified in SharedFiles::open & SharedFiles::close.
  • Loading branch information
eduar-hte committed Apr 25, 2024
1 parent f8942a8 commit 70a8bbd
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 284 deletions.
227 changes: 47 additions & 180 deletions src/utils/shared_files.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,236 +15,103 @@

#include "src/utils/shared_files.h"

#include <errno.h>
#include <fcntl.h>
#include <pthread.h>
#include <stdio.h>
#include <sys/ipc.h>
#include <sys/shm.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>
#include <stdlib.h>
#include <string.h>

#include <utility>
#include <fstream>
#include <string>
#ifdef WIN32
#include <windows.h>
#include <io.h>
#endif


namespace modsecurity {
namespace utils {


std::pair<msc_file_handler *, FILE *> SharedFiles::find_handler(
const std::string &fileName) {
for (const auto &i : m_handlers) {
if (i.first == fileName) {
return i.second;
}
}
return std::pair<modsecurity::utils::msc_file_handler *,
FILE *>(NULL, NULL);
}


std::pair<msc_file_handler *, FILE *> SharedFiles::add_new_handler(
SharedFiles::handlers_map::iterator SharedFiles::add_new_handler(
const std::string &fileName, std::string *error) {
int shm_id;
int ret;
key_t mem_key_structure;
msc_file_handler_t *new_debug_log = NULL;
struct shmid_ds shared_mem_info;
FILE *fp;
bool toBeCreated = true;

fp = fopen(fileName.c_str(), "a");
FILE *fp = fopen(fileName.c_str(), "a");
if (fp == 0) {
error->assign("Failed to open file: " + fileName);
goto err_fh;
}

mem_key_structure = ftok(fileName.c_str(), 1);
if (mem_key_structure < 0) {
error->assign("Failed to select key for the shared memory (1): ");
error->append(strerror(errno));
goto err_mem_key;
}

shm_id = shmget(mem_key_structure, sizeof (msc_file_handler_t) \
+ fileName.size() + 1, IPC_CREAT | IPC_EXCL | 0666);
if (shm_id < 0) {
shm_id = shmget(mem_key_structure, sizeof (msc_file_handler_t)
+ fileName.size() + 1, IPC_CREAT | 0666);
toBeCreated = false;
if (shm_id < 0) {
error->assign("Failed to allocate shared memory (1): ");
error->append(strerror(errno));
goto err_shmget1;
}
return m_handlers.end();
}

ret = shmctl(shm_id, IPC_STAT, &shared_mem_info);
if (ret < 0) {
error->assign("Failed to get information on shared memory (1): ");
error->append(strerror(errno));
goto err_shmctl1;
}

new_debug_log = reinterpret_cast<msc_file_handler_t *>(
shmat(shm_id, NULL, 0));
if ((reinterpret_cast<char *>(new_debug_log)[0]) == -1) {
error->assign("Failed to attach shared memory (1): ");
error->append(strerror(errno));
goto err_shmat1;
}

if (toBeCreated == false && shared_mem_info.shm_nattch == 0) {
toBeCreated = true;
}

if (toBeCreated) {
memset(new_debug_log, '\0', sizeof(msc_file_handler_t));
new_debug_log->shm_id_structure = shm_id;
memcpy(new_debug_log->file_name, fileName.c_str(), fileName.size());
new_debug_log->file_name[fileName.size()] = '\0';
}
m_handlers.push_back(std::make_pair(fileName,
std::make_pair(new_debug_log, fp)));

return std::make_pair(new_debug_log, fp);
err_shmat1:
shmdt(new_debug_log);
err_shmctl1:
err_shmget1:
err_mem_key:
fclose(fp);
err_fh:
return std::pair<modsecurity::utils::msc_file_handler *,
FILE *>(NULL, NULL);
return m_handlers.insert({ fileName, {fp, 0} }).first;
}


bool SharedFiles::open(const std::string& fileName, std::string *error) {
std::pair<msc_file_handler *, FILE *> a;
bool ret = true;

#if MODSEC_USE_GENERAL_LOCK
pthread_mutex_lock(m_generalLock);
#endif

a = find_handler(fileName);
if (a.first == NULL) {
a = add_new_handler(fileName, error);
if (error->size() > 0) {
ret = false;
goto out;
}
auto it = m_handlers.find(fileName);
if (it == m_handlers.end()) {
it = add_new_handler(fileName, error);
if (error->size() > 0)
return false;
}
if (a.first == NULL) {

if (it == m_handlers.end()) {
error->assign("Not able to open: " + fileName);
ret = false;
goto out;
return false;
}

out:
#if MODSEC_USE_GENERAL_LOCK
pthread_mutex_unlock(m_generalLock);
#endif
it->second.cnt++;

return ret;
return true;
}


void SharedFiles::close(const std::string& fileName) {
std::pair<msc_file_handler *, FILE *> a;
/* int ret; */
/* int shm_id; */
/* struct shmid_ds shared_mem_info; */
/* int j = 0; */

#if MODSEC_USE_GENERAL_LOCK
pthread_mutex_lock(m_generalLock);
#endif

if (fileName.empty()) {
goto out;
}

a = find_handler(fileName);
if (a.first == NULL || a.second == NULL) {
goto out;
}

/* fclose(a.second); */
a.second = 0;

/*
* Delete the file structure will be welcomed, but we cannot delay
* while the process is being killed.
*
for (std::pair<std::string,
std::pair<msc_file_handler *, FILE *>> i : m_handlers) {
if (i.first == fileName) {
j++;
}
}
if (fileName.empty())
return;

m_handlers.erase(m_handlers.begin()+j);
*/
auto it = m_handlers.find(fileName);
if (it == m_handlers.end())
return;

/* hmdt(a.second); */
shmctl(a.first->shm_id_structure, IPC_RMID, NULL);
it->second.cnt--;
if (it->second.cnt == 0)
{
fclose(it->second.fp);

/*
*
* We could check to see how many process attached to the shared memory
* we have, prior to the deletion of the shared memory.
*
ret = shmctl(a.first->shm_id_structure, IPC_STAT, &shared_mem_info);
if (ret < 0) {
goto out;
m_handlers.erase(it);
}
ret = shared_mem_info.shm_nattch;
shm_id = a.first->shm_id_structure;
*/

out:
#if MODSEC_USE_GENERAL_LOCK
pthread_mutex_unlock(m_generalLock);
#endif
return;
}


bool SharedFiles::write(const std::string& fileName,
const std::string &msg, std::string *error) {
std::pair<msc_file_handler *, FILE *> a;
std::string lmsg = msg;
size_t wrote;
struct flock lock{};
bool ret = true;

a = find_handler(fileName);
if (a.first == NULL) {
auto it = m_handlers.find(fileName);
if (it == m_handlers.end()) {
error->assign("file is not open: " + fileName);
return false;
}

//Exclusively lock whole file
#ifndef WIN32
struct flock lock {};
lock.l_start = lock.l_len = lock.l_whence = 0;
lock.l_type = F_WRLCK;
fcntl(fileno(a.second), F_SETLKW, &lock);
fcntl(fileno(it->second.fp), F_SETLKW, &lock);
#else
auto handle = reinterpret_cast<HANDLE>(_get_osfhandle(fileno(it->second.fp)));
OVERLAPPED overlapped = { 0 };
::LockFileEx(handle, LOCKFILE_EXCLUSIVE_LOCK, 0, MAXDWORD, MAXDWORD, &overlapped);
#endif

wrote = fwrite(lmsg.c_str(), 1, lmsg.size(), a.second);
auto wrote = fwrite(msg.c_str(), 1, msg.size(), it->second.fp);
if (wrote < msg.size()) {
error->assign("failed to write: " + fileName);
ret = false;
}
fflush(a.second);
fflush(it->second.fp);

//Remove exclusive lock
#ifndef WIN32
lock.l_type = F_UNLCK;
fcntl(fileno(a.second), F_SETLKW, &lock);
fcntl(fileno(it->second.fp), F_SETLKW, &lock);
#else
overlapped = { 0 };
::UnlockFileEx(handle, 0, MAXDWORD, MAXDWORD, &overlapped);
#endif

return ret;
}
Expand Down
Loading

0 comments on commit 70a8bbd

Please sign in to comment.