Skip to content

Commit dd0b32a

Browse files
diffstormdiffstorm
committed
Improve config parsing safety
1 parent 81263f8 commit dd0b32a

File tree

5 files changed

+181
-102
lines changed

5 files changed

+181
-102
lines changed

‎Makefile

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ $(DEPLOY_DIR):
3333
mkdir -p $(DEPLOY_DIR)
3434

3535
clean:
36-
rm -f $(DEPLOY_DIR)/$(TARGET_EXEC)
36+
rm -f $(DEPLOY_DIR)/$(TARGET_EXEC) $(DEPLOY_DIR)/stats_*.log $(DEPLOY_DIR)/stats_*.raw wdt.log
3737

3838
install: $(DEPLOY_DIR)/$(TARGET_EXEC)
3939
cp $(DEPLOY_DIR)/$(TARGET_EXEC) ~/

‎src/apps.c

Lines changed: 149 additions & 101 deletions
Original file line numberDiff line numberDiff line change
@@ -55,13 +55,18 @@ typedef struct
5555
time_t last_heartbeat; /**< Time when the last heartbeat was received from the application. */
5656
} Application_t;
5757

58+
typedef struct
59+
{
60+
int app_count; /**< Total number of applications found in the ini file. */
61+
int udp_port; /**< UDP port number specified in the ini file. */
62+
char ini_file[MAX_APP_CMD_LENGTH]; /**< Path to the ini file. */
63+
time_t ini_last_modified_time; /**< Last modified time of the ini file. */
64+
time_t uptime; /**< System uptime in seconds. */
65+
int ini_index; /**< Index used to read an array in the ini file. */
66+
} AppState_t;
67+
5868
static Application_t apps[MAX_APPS]; /**< Array of Application_t structures representing applications defined in the ini file. */
59-
static int app_count; /**< Total number of applications found in the ini file. */
60-
static int udp_port = 12345; /**< UDP port number specified in the ini file. */
61-
static char ini_file[MAX_APP_CMD_LENGTH] = INI_FILE; /**< Path to the ini file. */
62-
static time_t ini_last_modified_time; /**< Last modified time of the ini file. */
63-
static long uptime; /**< System uptime in seconds. */
64-
static int ini_index; /**< Index used to read an array in the ini file. */
69+
static AppState_t app_state = {0};
6570

6671
//------------------------------------------------------------------
6772

@@ -89,7 +94,7 @@ void update_heartbeat_time(int i)
8994

9095
int find_pid(int pid)
9196
{
92-
for(int i = 0; i < app_count; i++)
97+
for(int i = 0; i < app_state.app_count; i++)
9398
{
9499
if(0 < apps[i].pid && pid == apps[i].pid)
95100
{
@@ -137,157 +142,195 @@ bool get_first_heartbeat(int i)
137142

138143
//------------------------------------------------------------------
139144

140-
int set_ini_file(char *path)
145+
static time_t file_modified_time(char *path)
146+
{
147+
struct stat attr;
148+
stat(path, &attr);
149+
return attr.st_mtime;
150+
}
151+
152+
bool is_ini_updated()
141153
{
142-
int ret = 1; // fail
143-
int length = strlen(path);
154+
time_t file_last_modified_time = file_modified_time(app_state.ini_file);
155+
return (file_last_modified_time != app_state.ini_last_modified_time);
156+
}
144157

145-
if(NULL != path && 0 < length && MAX_APP_CMD_LENGTH > length)
158+
static int check_ini_file(char *path)
159+
{
160+
if(!path || strlen(path) == 0 || strlen(path) >= MAX_APP_CMD_LENGTH)
146161
{
147-
if(f_exist(path))
148-
{
149-
strncpy(ini_file, path, MAX_APP_CMD_LENGTH);
150-
LOGD("ini file has been set to %s", ini_file);
151-
ret = 0; // success
152-
}
162+
return 1;
153163
}
154164

155-
if(ret)
165+
if(!f_exist(path))
156166
{
157-
LOGE("Error setting ini file %s", path);
167+
return 1;
158168
}
159169

160-
return ret;
170+
return 0;
161171
}
162172

163-
static time_t file_modified_time(char *path)
173+
int set_ini_file(char *path)
164174
{
165-
struct stat attr;
166-
stat(path, &attr);
167-
return attr.st_mtime;
168-
}
175+
if(check_ini_file(path))
176+
{
177+
LOGE("Invalid path");
178+
return 1;
179+
}
169180

170-
bool is_ini_updated()
171-
{
172-
time_t file_last_modified_time = file_modified_time(ini_file);
173-
return (file_last_modified_time != ini_last_modified_time);
181+
snprintf(app_state.ini_file, MAX_APP_CMD_LENGTH, "%s", path);
182+
LOGD("INI file set to: %s", app_state.ini_file);
183+
return 0;
174184
}
175185

176186
static int handler(void *user, const char *section, const char *name, const char *value)
177187
{
178188
(void)(user);
179-
const char *_section = "processWatchdog";
180-
char b[INI_MAX_LINE];
181-
#define MATCH(s, n) strcmp(section, s) == 0 && strcmp(name, n) == 0
182-
#define SECTION(n, s) snprintf(b, sizeof(b) - 1, "%d_%s", n + 1, s);
189+
const char *target_section = "processWatchdog";
190+
char expected_name[INI_MAX_LINE];
183191

184-
if(MATCH(_section, "udp_port"))
192+
if(strcmp(section, target_section) != 0)
185193
{
186-
udp_port = atoi(value);
194+
return 1;
187195
}
188196

189-
if(MATCH(_section, "n_apps"))
197+
// Global parameters
198+
if(strcmp(name, "udp_port") == 0)
190199
{
191-
app_count = atoi(value);
200+
if(!parse_int(value, 1, 65535, &app_state.udp_port))
201+
{
202+
LOGE("Invalid UDP port: %s", value);
203+
return 0;
204+
}
205+
206+
return 1;
192207
}
193208

194-
if(app_count > 0)
209+
if(strcmp(name, "n_apps") == 0)
195210
{
196-
SECTION(ini_index, "name");
197-
198-
if(MATCH(_section, b))
211+
if(!parse_int(value, 0, MAX_APPS, &app_state.app_count))
199212
{
200-
int length = strlen(value);
213+
LOGE("Invalid n_apps: %s", value);
214+
return 0;
215+
}
201216

202-
if(length > MAX_APP_NAME_LENGTH)
203-
{
204-
LOGE("NAME is longer than %d", MAX_APP_NAME_LENGTH);
205-
}
217+
return 1;
218+
}
206219

207-
length = length > MAX_APP_NAME_LENGTH ? MAX_APP_NAME_LENGTH : length;
208-
strncpy(apps[ini_index].name, value, length);
209-
}
220+
// Application parameters
221+
int index = app_state.ini_index;
210222

211-
SECTION(ini_index, "start_delay");
223+
if(index >= app_state.app_count)
224+
{
225+
return 1;
226+
}
212227

213-
if(MATCH(_section, b))
214-
{
215-
apps[ini_index].start_delay = atoi(value);
216-
}
228+
#define GEN_NAME(field) snprintf(expected_name, sizeof(expected_name), "%d_%s", index + 1, field)
217229

218-
SECTION(ini_index, "heartbeat_delay");
230+
if(GEN_NAME("name"), strcmp(name, expected_name) == 0)
231+
{
232+
snprintf(apps[index].name, MAX_APP_NAME_LENGTH, "%s", value);
219233

220-
if(MATCH(_section, b))
234+
if(strlen(value) >= MAX_APP_NAME_LENGTH)
221235
{
222-
apps[ini_index].heartbeat_delay = atoi(value);
236+
LOGW("App %d name truncated", index);
223237
}
224-
225-
SECTION(ini_index, "heartbeat_interval");
226-
227-
if(MATCH(_section, b))
238+
}
239+
else if(GEN_NAME("start_delay"), strcmp(name, expected_name) == 0)
240+
{
241+
if(!parse_int(value, 0, INT_MAX, &apps[index].start_delay))
228242
{
229-
apps[ini_index].heartbeat_interval = atoi(value);
243+
LOGE("Invalid start_delay for app %d: %s", index, value);
244+
return 0;
230245
}
231-
232-
SECTION(ini_index, "cmd"); // this always must be the last one
233-
234-
if(MATCH(_section, b))
246+
}
247+
else if(GEN_NAME("heartbeat_delay"), strcmp(name, expected_name) == 0)
248+
{
249+
if(!parse_int(value, 0, INT_MAX, &apps[index].heartbeat_delay))
235250
{
236-
int length = strlen(value);
237-
238-
if(length > MAX_APP_CMD_LENGTH)
239-
{
240-
LOGE("CMD is longer than %d", MAX_APP_CMD_LENGTH);
241-
}
251+
LOGE("Invalid heartbeat_delay for app %d: %s", index, value);
252+
return 0;
253+
}
254+
}
255+
else if(GEN_NAME("heartbeat_interval"), strcmp(name, expected_name) == 0)
256+
{
257+
if(!parse_int(value, 0, INT_MAX, &apps[index].heartbeat_interval))
258+
{
259+
LOGE("Invalid heartbeat_interval for app %d: %s", index, value);
260+
return 0;
261+
}
262+
}
263+
else if(GEN_NAME("cmd"), strcmp(name, expected_name) == 0) // this always must be the last one
264+
{
265+
snprintf(apps[index].cmd, MAX_APP_CMD_LENGTH, "%s", value);
242266

243-
length = length > MAX_APP_CMD_LENGTH ? MAX_APP_CMD_LENGTH : length;
244-
strncpy(apps[ini_index].cmd, value, length);
245-
ini_index++; // order of the names in the ini are important
267+
if(strlen(value) >= MAX_APP_CMD_LENGTH)
268+
{
269+
LOGE("Invalid cmd for app %d - longer than %d charachters", index, MAX_APP_CMD_LENGTH);
270+
return 0;
246271
}
272+
273+
app_state.ini_index++; // Move to next app after processing cmd
247274
}
248275

249276
return 1;
250277
}
251278

252279
int read_ini_file()
253280
{
254-
uptime = get_uptime();
255-
LOGD("Reading ini file %s", ini_file);
256281
memset(apps, 0, sizeof(apps));
257-
app_count = 0;
258-
ini_index = 0;
282+
app_state.ini_index = 0;
283+
app_state.app_count = 0;
284+
app_state.uptime = get_uptime();
285+
app_state.udp_port = UDP_PORT;
259286

260-
if(ini_parse(ini_file, handler, NULL) < 0)
287+
// ini_file could have already set by set_ini_file() earlier
288+
if(check_ini_file(app_state.ini_file))
261289
{
262-
LOGE("Can't load %s", ini_file);
290+
LOGD("Using default ini file %s", INI_FILE);
291+
set_ini_file(INI_FILE);
292+
}
293+
294+
LOGD("Reading ini file %s", app_state.ini_file);
295+
296+
if(ini_parse(app_state.ini_file, handler, NULL) < 0)
297+
{
298+
LOGE("Failed to parse INI file %s", app_state.ini_file);
263299
return 1;
264300
}
265301

266-
LOGD("%d processes have found in the ini file %s", app_count, ini_file);
267-
ini_last_modified_time = file_modified_time(ini_file);
302+
if(app_state.ini_index != app_state.app_count)
303+
{
304+
LOGW("Config mismatch: Expected %d apps, found %d", app_state.app_count, app_state.ini_index);
305+
}
306+
307+
LOGD("%d processes have found in the ini file %s", app_state.app_count, app_state.ini_file);
308+
app_state.ini_last_modified_time = file_modified_time(app_state.ini_file);
268309
return 0;
269310
}
270311

271312
//------------------------------------------------------------------
272313

273314
bool is_application_running(int i)
274315
{
275-
if(apps[i].pid > 0)
316+
if(apps[i].pid <= 0)
276317
{
277-
// Check if the application is running on Linux
278-
if(kill(apps[i].pid, 0) == 0)
279-
{
280-
return true; // Process is running
281-
}
282-
else if(errno == EPERM)
283-
{
284-
LOGE("No permission to check if process %s is running : %s", apps[i].name, strerror(errno));
285-
return true;
286-
}
287-
else
288-
{
289-
LOGD("Process %s is not running : %s", apps[i].name, strerror(errno));
290-
}
318+
return false;
319+
}
320+
321+
// Check if the application is running
322+
if(kill(apps[i].pid, 0) == 0)
323+
{
324+
return true; // Process is running
325+
}
326+
else if(errno == EPERM)
327+
{
328+
LOGE("No permission to check if process %s is running : %s", apps[i].name, strerror(errno));
329+
return true;
330+
}
331+
else
332+
{
333+
LOGD("Process %s is not running : %s", apps[i].name, strerror(errno));
291334
}
292335

293336
return false;
@@ -300,7 +343,7 @@ bool is_application_started(int i)
300343

301344
bool is_application_start_time(int i)
302345
{
303-
return (get_uptime() - uptime) >= (long)apps[i].start_delay;
346+
return (get_uptime() - app_state.uptime) >= (long)apps[i].start_delay;
304347
}
305348

306349
void start_application(int i)
@@ -347,6 +390,11 @@ void kill_application(int i)
347390
bool killed = false;
348391
LOGD("Killing process %s", apps[i].name);
349392

393+
if(apps[i].pid <= 0)
394+
{
395+
return;
396+
}
397+
350398
if(kill(apps[i].pid, SIGTERM) < 0 && errno != ESRCH)
351399
{
352400
LOGE("Failed to terminate process %s, error: %d - %s", apps[i].name, errno, strerror(errno));
@@ -474,7 +522,7 @@ void restart_application(int i)
474522

475523
int get_app_count(void)
476524
{
477-
return app_count;
525+
return app_state.app_count;
478526
}
479527

480528
char *get_app_name(int i)
@@ -484,5 +532,5 @@ char *get_app_name(int i)
484532

485533
int get_udp_port(void)
486534
{
487-
return udp_port;
535+
return app_state.udp_port;
488536
}

‎src/apps.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
#define MAX_WAIT_PROCESS_START 5 /**< Maximum time to wait for a process to start running (seconds). */
3737
#define MAX_WAIT_PROCESS_TERMINATION 30 /**< Maximum time to wait for a process to terminate (seconds). */
3838
#define INI_FILE "config.ini" /**< Default ini file path. */
39+
#define UDP_PORT 12345 /**< Default udp port. */
3940

4041
// Function prototypes
4142

‎src/utils.c

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -368,6 +368,21 @@ int parse_number(const char *ptr, int length, char *cnt)
368368
return sum;
369369
}
370370

371+
bool parse_int(const char *ptr, int min_val, int max_val, int *result)
372+
{
373+
char *endptr;
374+
errno = 0;
375+
long tmp = strtol(ptr, &endptr, 10);
376+
377+
if(errno != 0 || *endptr != '\0' || tmp < min_val || tmp > max_val)
378+
{
379+
return false;
380+
}
381+
382+
*result = (int)tmp;
383+
return true;
384+
}
385+
371386
void substring(char s[], char sub[], int p, int l)
372387
{
373388
int c = 0;

0 commit comments

Comments
 (0)