Skip to content

Commit 70a8bbd

Browse files
committed
Added support to lock files on Windows and major rewrite to reintroduce 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.
1 parent f8942a8 commit 70a8bbd

File tree

2 files changed

+70
-284
lines changed

2 files changed

+70
-284
lines changed

src/utils/shared_files.cc

Lines changed: 47 additions & 180 deletions
Original file line numberDiff line numberDiff line change
@@ -15,236 +15,103 @@
1515

1616
#include "src/utils/shared_files.h"
1717

18-
#include <errno.h>
1918
#include <fcntl.h>
20-
#include <pthread.h>
21-
#include <stdio.h>
22-
#include <sys/ipc.h>
23-
#include <sys/shm.h>
24-
#include <sys/stat.h>
25-
#include <sys/types.h>
26-
#include <unistd.h>
27-
#include <stdlib.h>
28-
#include <string.h>
29-
30-
#include <utility>
31-
#include <fstream>
32-
#include <string>
19+
#ifdef WIN32
20+
#include <windows.h>
21+
#include <io.h>
22+
#endif
23+
3324

3425
namespace modsecurity {
3526
namespace utils {
3627

3728

38-
std::pair<msc_file_handler *, FILE *> SharedFiles::find_handler(
39-
const std::string &fileName) {
40-
for (const auto &i : m_handlers) {
41-
if (i.first == fileName) {
42-
return i.second;
43-
}
44-
}
45-
return std::pair<modsecurity::utils::msc_file_handler *,
46-
FILE *>(NULL, NULL);
47-
}
48-
49-
50-
std::pair<msc_file_handler *, FILE *> SharedFiles::add_new_handler(
29+
SharedFiles::handlers_map::iterator SharedFiles::add_new_handler(
5130
const std::string &fileName, std::string *error) {
52-
int shm_id;
53-
int ret;
54-
key_t mem_key_structure;
55-
msc_file_handler_t *new_debug_log = NULL;
56-
struct shmid_ds shared_mem_info;
57-
FILE *fp;
58-
bool toBeCreated = true;
59-
60-
fp = fopen(fileName.c_str(), "a");
31+
FILE *fp = fopen(fileName.c_str(), "a");
6132
if (fp == 0) {
6233
error->assign("Failed to open file: " + fileName);
63-
goto err_fh;
64-
}
65-
66-
mem_key_structure = ftok(fileName.c_str(), 1);
67-
if (mem_key_structure < 0) {
68-
error->assign("Failed to select key for the shared memory (1): ");
69-
error->append(strerror(errno));
70-
goto err_mem_key;
71-
}
72-
73-
shm_id = shmget(mem_key_structure, sizeof (msc_file_handler_t) \
74-
+ fileName.size() + 1, IPC_CREAT | IPC_EXCL | 0666);
75-
if (shm_id < 0) {
76-
shm_id = shmget(mem_key_structure, sizeof (msc_file_handler_t)
77-
+ fileName.size() + 1, IPC_CREAT | 0666);
78-
toBeCreated = false;
79-
if (shm_id < 0) {
80-
error->assign("Failed to allocate shared memory (1): ");
81-
error->append(strerror(errno));
82-
goto err_shmget1;
83-
}
34+
return m_handlers.end();
8435
}
8536

86-
ret = shmctl(shm_id, IPC_STAT, &shared_mem_info);
87-
if (ret < 0) {
88-
error->assign("Failed to get information on shared memory (1): ");
89-
error->append(strerror(errno));
90-
goto err_shmctl1;
91-
}
92-
93-
new_debug_log = reinterpret_cast<msc_file_handler_t *>(
94-
shmat(shm_id, NULL, 0));
95-
if ((reinterpret_cast<char *>(new_debug_log)[0]) == -1) {
96-
error->assign("Failed to attach shared memory (1): ");
97-
error->append(strerror(errno));
98-
goto err_shmat1;
99-
}
100-
101-
if (toBeCreated == false && shared_mem_info.shm_nattch == 0) {
102-
toBeCreated = true;
103-
}
104-
105-
if (toBeCreated) {
106-
memset(new_debug_log, '\0', sizeof(msc_file_handler_t));
107-
new_debug_log->shm_id_structure = shm_id;
108-
memcpy(new_debug_log->file_name, fileName.c_str(), fileName.size());
109-
new_debug_log->file_name[fileName.size()] = '\0';
110-
}
111-
m_handlers.push_back(std::make_pair(fileName,
112-
std::make_pair(new_debug_log, fp)));
113-
114-
return std::make_pair(new_debug_log, fp);
115-
err_shmat1:
116-
shmdt(new_debug_log);
117-
err_shmctl1:
118-
err_shmget1:
119-
err_mem_key:
120-
fclose(fp);
121-
err_fh:
122-
return std::pair<modsecurity::utils::msc_file_handler *,
123-
FILE *>(NULL, NULL);
37+
return m_handlers.insert({ fileName, {fp, 0} }).first;
12438
}
12539

12640

12741
bool SharedFiles::open(const std::string& fileName, std::string *error) {
128-
std::pair<msc_file_handler *, FILE *> a;
129-
bool ret = true;
130-
131-
#if MODSEC_USE_GENERAL_LOCK
132-
pthread_mutex_lock(m_generalLock);
133-
#endif
134-
135-
a = find_handler(fileName);
136-
if (a.first == NULL) {
137-
a = add_new_handler(fileName, error);
138-
if (error->size() > 0) {
139-
ret = false;
140-
goto out;
141-
}
42+
auto it = m_handlers.find(fileName);
43+
if (it == m_handlers.end()) {
44+
it = add_new_handler(fileName, error);
45+
if (error->size() > 0)
46+
return false;
14247
}
143-
if (a.first == NULL) {
48+
49+
if (it == m_handlers.end()) {
14450
error->assign("Not able to open: " + fileName);
145-
ret = false;
146-
goto out;
51+
return false;
14752
}
14853

149-
out:
150-
#if MODSEC_USE_GENERAL_LOCK
151-
pthread_mutex_unlock(m_generalLock);
152-
#endif
54+
it->second.cnt++;
15355

154-
return ret;
56+
return true;
15557
}
15658

15759

15860
void SharedFiles::close(const std::string& fileName) {
159-
std::pair<msc_file_handler *, FILE *> a;
160-
/* int ret; */
161-
/* int shm_id; */
162-
/* struct shmid_ds shared_mem_info; */
163-
/* int j = 0; */
164-
165-
#if MODSEC_USE_GENERAL_LOCK
166-
pthread_mutex_lock(m_generalLock);
167-
#endif
168-
169-
if (fileName.empty()) {
170-
goto out;
171-
}
172-
173-
a = find_handler(fileName);
174-
if (a.first == NULL || a.second == NULL) {
175-
goto out;
176-
}
177-
178-
/* fclose(a.second); */
179-
a.second = 0;
180-
181-
/*
182-
* Delete the file structure will be welcomed, but we cannot delay
183-
* while the process is being killed.
184-
*
185-
for (std::pair<std::string,
186-
std::pair<msc_file_handler *, FILE *>> i : m_handlers) {
187-
if (i.first == fileName) {
188-
j++;
189-
}
190-
}
61+
if (fileName.empty())
62+
return;
19163

192-
m_handlers.erase(m_handlers.begin()+j);
193-
*/
64+
auto it = m_handlers.find(fileName);
65+
if (it == m_handlers.end())
66+
return;
19467

195-
/* hmdt(a.second); */
196-
shmctl(a.first->shm_id_structure, IPC_RMID, NULL);
68+
it->second.cnt--;
69+
if (it->second.cnt == 0)
70+
{
71+
fclose(it->second.fp);
19772

198-
/*
199-
*
200-
* We could check to see how many process attached to the shared memory
201-
* we have, prior to the deletion of the shared memory.
202-
*
203-
ret = shmctl(a.first->shm_id_structure, IPC_STAT, &shared_mem_info);
204-
if (ret < 0) {
205-
goto out;
73+
m_handlers.erase(it);
20674
}
207-
ret = shared_mem_info.shm_nattch;
208-
shm_id = a.first->shm_id_structure;
209-
*/
210-
211-
out:
212-
#if MODSEC_USE_GENERAL_LOCK
213-
pthread_mutex_unlock(m_generalLock);
214-
#endif
215-
return;
21675
}
21776

21877

21978
bool SharedFiles::write(const std::string& fileName,
22079
const std::string &msg, std::string *error) {
221-
std::pair<msc_file_handler *, FILE *> a;
222-
std::string lmsg = msg;
223-
size_t wrote;
224-
struct flock lock{};
22580
bool ret = true;
22681

227-
a = find_handler(fileName);
228-
if (a.first == NULL) {
82+
auto it = m_handlers.find(fileName);
83+
if (it == m_handlers.end()) {
22984
error->assign("file is not open: " + fileName);
23085
return false;
23186
}
23287

23388
//Exclusively lock whole file
89+
#ifndef WIN32
90+
struct flock lock {};
23491
lock.l_start = lock.l_len = lock.l_whence = 0;
23592
lock.l_type = F_WRLCK;
236-
fcntl(fileno(a.second), F_SETLKW, &lock);
93+
fcntl(fileno(it->second.fp), F_SETLKW, &lock);
94+
#else
95+
auto handle = reinterpret_cast<HANDLE>(_get_osfhandle(fileno(it->second.fp)));
96+
OVERLAPPED overlapped = { 0 };
97+
::LockFileEx(handle, LOCKFILE_EXCLUSIVE_LOCK, 0, MAXDWORD, MAXDWORD, &overlapped);
98+
#endif
23799

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

245107
//Remove exclusive lock
108+
#ifndef WIN32
246109
lock.l_type = F_UNLCK;
247-
fcntl(fileno(a.second), F_SETLKW, &lock);
110+
fcntl(fileno(it->second.fp), F_SETLKW, &lock);
111+
#else
112+
overlapped = { 0 };
113+
::UnlockFileEx(handle, 0, MAXDWORD, MAXDWORD, &overlapped);
114+
#endif
248115

249116
return ret;
250117
}

0 commit comments

Comments
 (0)