Skip to content

Commit d8a4561

Browse files
MartinPulecMartinPulec
committed
module: do not depend on storage provided by caller
The caller might have deleted the state without calling module_done and then the dangling pointer is referenced.
1 parent ab5d2a5 commit d8a4561

File tree

2 files changed

+53
-52
lines changed

2 files changed

+53
-52
lines changed

‎src/module.c‎

Lines changed: 46 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -67,30 +67,31 @@ module_mutex_unlock(pthread_mutex_t *lock)
6767

6868
void module_register(struct module *module_data, struct module *parent)
6969
{
70-
module_data->module_priv = calloc(1, sizeof *module_data->module_priv);
71-
module_data->module_priv->cls = module_data->cls;
72-
module_data->module_priv->magic = MODULE_MAGIC;
70+
struct module_priv_state *module_priv = calloc(1, sizeof *module_data->module_priv);
71+
module_data->module_priv = module_priv;
72+
module_priv->magic = MODULE_MAGIC;
73+
memcpy(&module_priv->wrapper, module_data, sizeof *module_data);
7374

7475
int ret = 0;
7576
pthread_mutexattr_t attr;
7677
ret |= pthread_mutexattr_init(&attr);
7778
ret |= pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE);
78-
ret |= pthread_mutex_init(&module_data->module_priv->lock, &attr);
79-
ret |= pthread_mutex_init(&module_data->module_priv->msg_queue_lock, &attr);
79+
ret |= pthread_mutex_init(&module_priv->lock, &attr);
80+
ret |= pthread_mutex_init(&module_priv->msg_queue_lock, &attr);
8081
ret |= pthread_mutexattr_destroy(&attr);
8182
assert(ret == 0 && "Unable to create mutex or set attributes");
8283

83-
module_data->module_priv->children = simple_linked_list_init();
84-
module_data->module_priv->msg_queue = simple_linked_list_init();
85-
module_data->module_priv->msg_queue_children = simple_linked_list_init();
84+
module_priv->children = simple_linked_list_init();
85+
module_priv->msg_queue = simple_linked_list_init();
86+
module_priv->msg_queue_children = simple_linked_list_init();
8687

8788
// register to parent
8889
if (parent == NULL) {
8990
return;
9091
}
91-
module_data->module_priv->parent = parent;
92+
module_priv->parent = parent->module_priv;
9293
module_mutex_lock(&parent->module_priv->lock);
93-
simple_linked_list_append(parent->module_priv->children, module_data);
94+
simple_linked_list_append(parent->module_priv->children, module_priv);
9495
module_check_undelivered_messages(parent);
9596
module_mutex_unlock(&parent->module_priv->lock);
9697
}
@@ -108,11 +109,11 @@ void module_done(struct module *module_data)
108109
assert(module_priv->magic == MODULE_MAGIC);
109110

110111
if(module_priv->parent) {
111-
module_mutex_lock(&module_priv->parent->module_priv->lock);
112+
module_mutex_lock(&module_priv->parent->lock);
112113
bool found = simple_linked_list_remove(
113-
module_priv->parent->module_priv->children, module_data);
114+
module_priv->parent->children, module_data->module_priv);
114115
assert(found);
115-
module_mutex_unlock(&module_priv->parent->module_priv->lock);
116+
module_mutex_unlock(&module_priv->parent->lock);
116117
}
117118

118119
module_data->cls = MODULE_CLASS_NONE;
@@ -122,10 +123,11 @@ void module_done(struct module *module_data)
122123
dump_tree(module_data, 0);
123124
module_mutex_lock(&module_priv->lock);
124125
for(void *it = simple_linked_list_it_init(module_priv->children); it != NULL; ) {
125-
struct module *child = simple_linked_list_it_next(&it);
126-
module_mutex_lock(&child->module_priv->lock);
127-
child->module_priv->parent = NULL;
128-
module_mutex_unlock(&child->module_priv->lock);
126+
struct module_priv_state *child = simple_linked_list_it_next(&it);
127+
assert(child->magic == MODULE_MAGIC);
128+
module_mutex_lock(&child->lock);
129+
child->parent = NULL;
130+
module_mutex_unlock(&child->lock);
129131
}
130132
module_mutex_unlock(&module_priv->lock);
131133
}
@@ -197,20 +199,26 @@ append_message_path(char *buf, int buflen, const enum module_class *modules)
197199
}
198200
}
199201

200-
struct module *get_root_module(struct module *node)
202+
struct module *get_root_module(struct module *mod)
201203
{
202-
assert(node);
203-
while(node->module_priv->parent) {
204-
node = node->module_priv->parent;
204+
assert(mod && mod->module_priv);
205+
206+
struct module_priv_state *node = mod->module_priv;
207+
208+
while(node->parent) {
209+
node = node->parent;
205210
}
206-
assert(node->cls == MODULE_CLASS_ROOT);
211+
assert(node->wrapper.cls == MODULE_CLASS_ROOT);
207212

208-
return node;
213+
return &node->wrapper;
209214
}
210215

211216
struct module *get_parent_module(struct module *node)
212217
{
213-
return node->module_priv->parent;
218+
if (node->module_priv->parent == NULL) {
219+
return NULL;
220+
}
221+
return &node->module_priv->parent->wrapper;
214222
}
215223

216224
/**
@@ -222,18 +230,18 @@ static struct module *find_child(struct module *node, const char *node_name, int
222230
{
223231
for (void *it = simple_linked_list_it_init(node->module_priv->children);
224232
it != NULL;) {
225-
struct module *child = (struct module *) simple_linked_list_it_next(&it);
226-
const char *child_name = module_class_name(child->cls);
233+
struct module_priv_state *child = simple_linked_list_it_next(&it);
234+
const char *child_name = module_class_name(child->wrapper.cls);
227235
assert(child_name != NULL);
228236
if(strcasecmp(child_name, node_name) == 0) {
229237
if (id_name != NULL) {
230-
if (strcmp(child->name, id_name) == 0) {
238+
if (strcmp(child->wrapper.name, id_name) == 0) {
231239
simple_linked_list_it_destroy(it);
232-
return child;
240+
return &child->wrapper;
233241
}
234242
} else if (id_num-- == 0) {
235243
simple_linked_list_it_destroy(it);
236-
return child;
244+
return &child->wrapper;
237245
}
238246
}
239247
}
@@ -338,8 +346,8 @@ void dump_tree(struct module *root_node, int indent) {
338346
for (void *it =
339347
simple_linked_list_it_init(root_node->module_priv->children);
340348
it != NULL;) {
341-
struct module *child = simple_linked_list_it_next(&it);
342-
dump_tree(child, indent + 2);
349+
struct module_priv_state *child = simple_linked_list_it_next(&it);
350+
dump_tree(&child->wrapper, indent + 2);
343351
}
344352
module_mutex_unlock(&root_node->module_priv->lock);
345353
}
@@ -376,11 +384,11 @@ static const char *get_module_identifier(struct module *mod)
376384
int our_index = 0;
377385
for (void *it = simple_linked_list_it_init(parent->module_priv->children);
378386
it != NULL;) {
379-
struct module *child = simple_linked_list_it_next(&it);
380-
if (child->cls != mod->cls) {
387+
struct module_priv_state *child = simple_linked_list_it_next(&it);
388+
if (child->wrapper.cls != mod->cls) {
381389
continue;
382390
}
383-
if (child == mod) { // found our node
391+
if (child == mod->module_priv) { // found our node
384392
break;
385393
}
386394
our_index += 1;
@@ -409,8 +417,9 @@ bool module_get_path_str(struct module *mod, char *buf, size_t buflen) {
409417
assert(buflen > 0);
410418
buf[0] = '\0';
411419

412-
while (mod) {
413-
const char *cur_name = get_module_identifier(mod);
420+
struct module_priv_state *node = mod->module_priv;
421+
while (node) {
422+
const char *cur_name = get_module_identifier(&node->wrapper);
414423
if (sizeof(buf) + 1 + sizeof(cur_name) >= buflen) {
415424
return false;
416425
}
@@ -422,7 +431,7 @@ bool module_get_path_str(struct module *mod, char *buf, size_t buflen) {
422431
buf[strlen(cur_name)] = '\0';
423432
}
424433
memcpy(buf, cur_name, strlen(cur_name));
425-
mod = mod->module_priv->parent;
434+
node = node->parent;
426435
}
427436

428437
return true;

‎src/module.h‎

Lines changed: 7 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -111,25 +111,15 @@ struct module {
111111
///< module (control_socket)
112112
char name[128]; ///< optional name of the module. May be used for indexing.
113113

114-
struct module_priv_state *module_priv;
115-
116-
#ifdef __cplusplus
117-
module() = default;
118-
// don't be tempted to copy/move module because parent holds pointer to struct module
119-
// which would be dangling thereafter
120-
module(module const &) = delete;
121-
module(module &&) = delete;
122-
module &operator=(module const &) = delete;
123-
module &operator=(module &&) = delete;
124-
#endif
114+
struct module_priv_state *module_priv; ///< set by module_register()
125115
};
126116

127117
struct module_priv_state {
128118
uint32_t magic;
129-
enum module_class cls;
119+
struct module wrapper;
130120
pthread_mutex_t lock;
131-
struct module *parent;
132-
struct simple_linked_list *children;
121+
struct module_priv_state *parent;
122+
struct simple_linked_list *children; // module_priv_state
133123

134124
pthread_mutex_t msg_queue_lock; // protects msg_queue
135125
struct simple_linked_list *msg_queue;
@@ -140,6 +130,8 @@ struct module_priv_state {
140130
};
141131

142132
void module_init_default(struct module *module_data);
133+
/// module_register makes a private copy struct module so subsequent
134+
/// changes in that structure won't affect the registered one
143135
void module_register(struct module *module_data, struct module *parent);
144136
void module_done(struct module *module_data);
145137
const char *module_class_name(enum module_class cls);
@@ -186,7 +178,7 @@ struct module *get_matching_child(struct module *node, const char *path);
186178
*
187179
* @retval root module
188180
*/
189-
struct module *get_root_module(struct module *node);
181+
struct module *get_root_module(struct module *mod);
190182

191183
struct module *get_parent_module(struct module *node);
192184

0 commit comments

Comments
 (0)