[PATCH v3 0/3] Fix env, type, security and error handling

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

This patch series is an extended version 3 of the former
[PATCH v2] Fix type and security bugs

* If a new environment is created by the API, the user
expects a clone of the current environment except revision
and ustate rather than an empty environment with only
revision and ustate set.
The function ebg_env_create_new is altered so that it
creates a clone of the latest environment.

* Several compiler warnings, memory leaks, missing error
handling, unhandled return values, etc. are fixed.

The 2nd commit casts away warnings regarding ignored return
values as it focuses on all cppcheck warnings.

The 3rd commit adds propper error handling where return values
were ignored before as it focuses on memory allocation,
regarding malloc, calloc and asprintf.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>

Andreas Reichel (3):
env_api: Clone old environment on ebg_env_create_new
Fix type and security bugs
Fix mem alloc error handling in core and tools

.travis-build.sh | 10 ++++-
Makefile.am | 3 +-
env/env_api.c | 11 ++++-
env/env_api_fat.c | 16 +++----
env/uservars.c | 8 ++--
tools/bg_setenv.c | 118 ++++++++++++++++++++++++++++++++-----------------
tools/ebgpart.c | 92 ++++++++++++++++++++++++++------------
tools/tests/test_api.c | 3 +-
8 files changed, 173 insertions(+), 88 deletions(-)

--
2.14.1

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

User expects old values to be taken over when none are
specified i.e. in swupdate's sw-description. Thus behavior
of API is changed so that ebg_env_create_new takes the old
values except ustate and revision and copies them.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---
env/env_api.c | 8 ++++++++
tools/tests/test_api.c | 3 +--
2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/env/env_api.c b/env/env_api.c
index b3377fc..70e6241 100644
--- a/env/env_api.c
+++ b/env/env_api.c
@@ -66,7 +66,15 @@ int ebg_env_create_new(ebgenv_t *e)
}

if (!e->ebg_new_env_created) {
+ BGENV *latest_env = bgenv_open_latest();
e->bgenv = (void *)bgenv_create_new();
+ BG_ENVDATA *new_data = ((BGENV *)e->bgenv)->data;
+ uint32_t new_rev = new_data->revision;
+ uint8_t new_ustate = new_data->ustate;
+ memcpy(new_data, latest_env->data, sizeof(BG_ENVDATA));
+ new_data->revision = new_rev;
+ new_data->ustate = new_ustate;
+ bgenv_close(latest_env);
}

e->ebg_new_env_created = e->bgenv != NULL;
diff --git a/tools/tests/test_api.c b/tools/tests/test_api.c
index 37247ff..fc5267d 100644
--- a/tools/tests/test_api.c
+++ b/tools/tests/test_api.c
@@ -149,11 +149,10 @@ static void test_api_update(void **state)
{
will_return(bgenv_open_latest, &env);
will_return(bgenv_open_oldest, &envupdate);
+ will_return(bgenv_open_latest, &env);
assert_int_equal(ebg_env_create_new(&e), 0);

assert_int_equal(envupdate.data->revision, test_env_revision + 1);
- assert_int_equal(envupdate.data->watchdog_timeout_sec,
- DEFAULT_WATCHDOG_TIMEOUT_SEC);
assert_int_equal(envupdate.data->ustate, 1);

assert_int_equal(ebg_env_set(&e, "ustate", "2"), 0);
--
2.14.1

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

* Fix missing format strings to avoid exploits or stack corruption
* Fix warnings:
* missing type casts
* missing return values
* ignored return values
* possibly unitialized variables
* unused variables
* unintended integer overflows
* dereference of freed pointers
* Fix file offset size to 64 bits in Makefile

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---

.travis-build.sh | 10 +++++++-
Makefile.am | 3 ++-
env/env_api.c | 3 +--
env/env_api_fat.c | 16 ++++++------
env/uservars.c | 8 +++---
tools/bg_setenv.c | 77 +++++++++++++++++++++++++++++++------------------------
tools/ebgpart.c | 57 +++++++++++++++++++++-------------------
7 files changed, 99 insertions(+), 75 deletions(-)

diff --git a/.travis-build.sh b/.travis-build.sh
index bf71b6b..c4f0a65 100755
--- a/.travis-build.sh
+++ b/.travis-build.sh
@@ -109,6 +109,14 @@ case "$TARGET_EFFECTIVE" in
suppress+=" --suppress=variableScope:/usr/include/bits/stdlib-bsearch.h"
# Function 'efi_main' is called by efi:
suppress+=" --suppress=unusedFunction:main.c"
+ # Some functions are defined for API only
+ suppress+=" --suppress=unusedFunction:utils.c"
+ suppress+=" --suppress=unusedFunction:env/env_api.c"
+ suppress+=" --suppress=unusedFunction:env/fatvars.c"
+ suppress+=" --suppress=unusedFunction:tools/tests/test_environment.c"
+ suppress+=" --suppress=unusedFunction:env/env_api_fat.c"
+ # EFI uses void* as ImageBase needed for further calculations
+ suppress+=" --suppress=arithOperationsOnVoidPointer:main.c"

enable="--enable=warning \
--enable=style \
@@ -128,7 +136,7 @@ case "$TARGET_EFFECTIVE" in
cpp_conf="-U__WINT_TYPE__"
# Exit code '1' is returned if arguments are not valid or if no input
# files are provided. Compare 'cppcheck --help'.
- exec cppcheck -f -q --error-exitcode=2 --std=posix \
+ exec cppcheck -f -q --error-exitcode=2 \
$enable $suppress $cpp_conf $includes .
;;
coverity_prepare)
diff --git a/Makefile.am b/Makefile.am
index b11366d..51e2658 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -27,7 +27,8 @@ AM_CFLAGS = \
-Wmissing-prototypes \
-fshort-wchar \
-DHAVE_ENDIAN_H \
- -D_GNU_SOURCE
+ -D_GNU_SOURCE \
+ -D_FILE_OFFSET_BITS=64

AM_LDFLAGS = -L$(top_builddir)/

diff --git a/env/env_api.c b/env/env_api.c
index 70e6241..8ee881a 100644
--- a/env/env_api.c
+++ b/env/env_api.c
@@ -145,7 +145,6 @@ uint16_t ebg_env_getglobalstate(ebgenv_t *e)
* with ustate == USTATE_FAILED */
if (env->data->revision == REVISION_FAILED &&
env->data->ustate == USTATE_FAILED) {
- (void)bgenv_close(env);
res = 3;
}
(void)bgenv_close(env);
@@ -174,7 +173,7 @@ int ebg_env_setglobalstate(ebgenv_t *e, uint16_t ustate)
if (ustate > USTATE_FAILED) {
return EINVAL;
}
- snprintf(buffer, sizeof(buffer), "%d", ustate);
+ (void)snprintf(buffer, sizeof(buffer), "%d", ustate);
res =
bgenv_set((BGENV *)e->bgenv, "ustate", "String", buffer,
strlen(buffer));
diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index a16db61..354d7f8 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -78,7 +78,7 @@ bool mount_partition(CONFIG_PART *cfgpart)
{
char tmpdir_template[256];
char *mountpoint;
- snprintf(tmpdir_template, 256, "%s", tmp_mnt_dir);
+ (void)snprintf(tmpdir_template, 256, "%s", tmp_mnt_dir);
if (!cfgpart) {
return false;
}
@@ -204,7 +204,7 @@ bool probe_config_partitions(CONFIG_PART *cfgpart)

ped_device_probe_all();

- while (dev = ped_device_get_next(dev)) {
+ while ((dev = ped_device_get_next(dev))) {
printf_debug("Device: %s\n", dev->model);
PedDisk *pd = ped_disk_new(dev);
if (!pd) {
@@ -220,11 +220,11 @@ bool probe_config_partitions(CONFIG_PART *cfgpart)
continue;
}
if (strncmp("/dev/mmcblk", dev->path, 11) == 0) {
- snprintf(devpath, 4096, "%sp%u", dev->path,
- part->num);
+ (void)snprintf(devpath, 4096, "%sp%u",
+ dev->path, part->num);
} else {
- snprintf(devpath, 4096, "%s%u", dev->path,
- part->num);
+ (void)snprintf(devpath, 4096, "%s%u",
+ dev->path, part->num);
}
if (!cfgpart[count].devpath) {
cfgpart[count].devpath =
@@ -493,7 +493,7 @@ int bgenv_get(BGENV *env, char *key, char *type, void *data, uint32_t maxlen)
}
break;
case EBGENV_WATCHDOG_TIMEOUT_SEC:
- sprintf(buffer, "%lu", env->data->watchdog_timeout_sec);
+ sprintf(buffer, "%u", env->data->watchdog_timeout_sec);
if (!data) {
return strlen(buffer);
}
@@ -503,7 +503,7 @@ int bgenv_get(BGENV *env, char *key, char *type, void *data, uint32_t maxlen)
}
break;
case EBGENV_REVISION:
- sprintf(buffer, "%lu", env->data->revision);
+ sprintf(buffer, "%u", env->data->revision);
if (!data) {
return strlen(buffer);
}
diff --git a/env/uservars.c b/env/uservars.c
index ffdd604..abbcf54 100644
--- a/env/uservars.c
+++ b/env/uservars.c
@@ -69,7 +69,7 @@ void bgenv_serialize_uservar(uint8_t *p, char *key, char *type, void *data,
uint32_t payload_size, data_size;

/* store key */
- strncpy(p, key, strlen(key) + 1);
+ strncpy((char *)p, key, strlen(key) + 1);
p += strlen(key) + 1;

/* store payload_size after key */
@@ -180,8 +180,8 @@ uint8_t *bgenv_uservar_alloc(uint8_t *udata, uint32_t datalen)
return NULL;
}
spaceleft = bgenv_user_free(udata);
- VERBOSE(stdout, "uservar_alloc: free: %u requested: %u \n", spaceleft,
- datalen);
+ VERBOSE(stdout, "uservar_alloc: free: %lu requested: %lu \n",
+ (unsigned long)spaceleft, (unsigned long)datalen);

/* To find the end of user variables, a 2nd 0 must be there after the
* last variable content, thus, we need one extra byte if appending a
@@ -258,7 +258,7 @@ uint32_t bgenv_user_free(uint8_t *udata)
while (*udata) {
bgenv_map_uservar(udata, NULL, NULL, NULL, &rsize, NULL);
spaceleft -= rsize;
- if (spaceleft <= 0)
+ if (spaceleft == 0)
break;
udata = bgenv_next_uservar(udata);
}
diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index 424b442..31f0c61 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -73,10 +73,10 @@ static void journal_add_action(BGENV_TASK task, char *key, char *type,
new_action = calloc(1, sizeof(struct env_action));
new_action->task = task;
if (key) {
- asprintf(&(new_action->key), "%s", key);
+ (void)asprintf(&(new_action->key), "%s", key);
}
if (type) {
- asprintf(&(new_action->type), "%s", type);
+ (void)asprintf(&(new_action->type), "%s", type);
}
if (data && datalen) {
new_action->data = (uint8_t *)malloc(datalen);
@@ -96,34 +96,38 @@ static void journal_process_action(BGENV *env, struct env_action *action)
{
uint8_t *var;
ebgenv_t e;
- uint16_t ustate;
- char *arg, *tmp;
- int ret;
+ char *tmp;

switch (action->task) {
case ENV_TASK_SET:
VERBOSE(stdout, "Task = SET, key = %s, type = %s, val = %s\n",
action->key, action->type, (char *)action->data);
if (strncmp(action->key, "ustate", strlen("ustate")+1) == 0) {
+ uint16_t ustate;
+ unsigned long t;
+ char *arg;
+ int ret;
e.bgenv = env;
arg = (char *)action->data;
errno = 0;
- ustate = strtol(arg, &tmp, 10);
- if ((errno == ERANGE && (ustate == LONG_MAX ||
- ustate == LONG_MIN)) ||
- (errno != 0 && ustate == 0) || (tmp == arg)) {
+ t = strtol(arg, &tmp, 10);
+ if ((errno == ERANGE && (t == LONG_MAX ||
+ t == LONG_MIN)) ||
+ (errno != 0 && t == 0) || (tmp == arg)) {
fprintf(stderr, "Invalid value for ustate: %s",
(char *)action->data);
return;
}
- if (ret = ebg_env_setglobalstate(&e, ustate) != 0) {
- fprintf(stderr, "Error setting global state.",
+ ustate = (uint16_t)t;;
+ if ((ret = ebg_env_setglobalstate(&e, ustate)) != 0) {
+ fprintf(stderr,
+ "Error setting global state: %s.",
strerror(ret));
}
return;
}
bgenv_set(env, action->key, action->type, action->data,
- strlen(action->data) + 1);
+ strlen((char *)action->data) + 1);
break;
case ENV_TASK_DEL:
VERBOSE(stdout, "Task = DEL, key = %s\n", action->key);
@@ -165,9 +169,10 @@ static uint8_t str2ustate(char *str)

static char *ustate2str(uint8_t ustate)
{
- if (ustate >= USTATE_MIN && ustate <= USTATE_MAX) {
- return ustatemap[ustate];
+ if (ustate > USTATE_MAX) {
+ ustate = USTATE_MAX;
}
+ return ustatemap[ustate];
}

static int set_uservars(char *arg)
@@ -185,8 +190,8 @@ static int set_uservars(char *arg)
return 0;
}

- journal_add_action(ENV_TASK_SET, key, USERVAR_TYPE_DEFAULT, value,
- strlen(value) + 1);
+ journal_add_action(ENV_TASK_SET, key, USERVAR_TYPE_DEFAULT,
+ (uint8_t *)value, strlen(value) + 1);

return 0;
}
@@ -195,7 +200,6 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
{
struct arguments *arguments = state->input;
int i;
- wchar_t buffer[ENV_STRING_LENGTH];
char *tmp;

switch (key) {
@@ -207,8 +211,8 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
ENV_STRING_LENGTH);
return 1;
}
- journal_add_action(ENV_TASK_SET, "kernelfile", "String", arg,
- strlen(arg) + 1);
+ journal_add_action(ENV_TASK_SET, "kernelfile", "String",
+ (uint8_t *)arg, strlen(arg) + 1);
break;
case 'a':
if (strlen(arg) > ENV_STRING_LENGTH) {
@@ -218,8 +222,8 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
ENV_STRING_LENGTH);
return 1;
}
- journal_add_action(ENV_TASK_SET, "kernelparams", "String", arg,
- strlen(arg) + 1);
+ journal_add_action(ENV_TASK_SET, "kernelparams", "String",
+ (uint8_t *)arg, strlen(arg) + 1);
break;
case 'p':
i = strtol(arg, &tmp, 10);
@@ -259,18 +263,18 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
ustatemap[3]);
return 1;
} else {
- asprintf(&tmp, "%u", i);
+ (void)asprintf(&tmp, "%u", i);
journal_add_action(ENV_TASK_SET, "ustate", "String",
- tmp, strlen(tmp) + 1);
- VERBOSE(stdout, "Ustate set to %u (%s).\n", i,
+ (uint8_t *)tmp, strlen(tmp) + 1);
+ VERBOSE(stdout, "Ustate set to %d (%s).\n", i,
ustate2str(i));
}
break;
case 'r':
i = atoi(arg);
VERBOSE(stdout, "Revision is set to %d.\n", i);
- journal_add_action(ENV_TASK_SET, "revision", "String", arg,
- strlen(arg) + 1);
+ journal_add_action(ENV_TASK_SET, "revision", "String",
+ (uint8_t *)arg, strlen(arg) + 1);
break;
case 'w':
i = atoi(arg);
@@ -278,7 +282,8 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
VERBOSE(stdout,
"Setting watchdog timeout to %d seconds.\n", i);
journal_add_action(ENV_TASK_SET, "watchdog_timeout_sec",
- "String", arg, strlen(arg) + 1);
+ "String", (uint8_t *)arg,
+ strlen(arg) + 1);
} else {
fprintf(stderr, "Watchdog timeout must be non-zero.\n");
return 1;
@@ -286,13 +291,14 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
break;
case 'f':
arguments->output_to_file = true;
- asprintf(&envfilepath, "%s/%s", arg, FAT_ENV_FILENAME);
+ (void)asprintf(&envfilepath, "%s/%s", arg, FAT_ENV_FILENAME);
break;
case 'c':
VERBOSE(stdout,
"Confirming environment to work. Removing boot-once "
"and testing flag.\n");
- journal_add_action(ENV_TASK_SET, "ustate", "String", "0", 2);
+ journal_add_action(ENV_TASK_SET, "ustate", "String",
+ (uint8_t *)"0", 2);
break;
case 'u':
if (part_specified) {
@@ -373,11 +379,16 @@ static void update_environment(BGENV *env)
journal_process_action(env, action);
journal_free_action(action);
STAILQ_REMOVE(&head, action, env_action, journal);
- free(action);
}

env->data->crc32 = crc32(0, (const Bytef *)env->data,
sizeof(BG_ENVDATA) - sizeof(env->data->crc32));
+
+ while (!STAILQ_EMPTY(&head)) {
+ action = STAILQ_FIRST(&head);
+ STAILQ_REMOVE_HEAD(&head, journal);
+ free(action);
+ }
}

static void dump_envs(void)
@@ -424,7 +435,8 @@ int main(int argc, char **argv)
STAILQ_INIT(&head);

error_t e;
- if (e = argp_parse(argp, argc, argv, 0, 0, &arguments)) {
+ e = argp_parse(argp, argc, argv, 0, 0, &arguments);
+ if (e) {
return e;
}

@@ -456,7 +468,7 @@ int main(int argc, char **argv)
}
if (fclose(of)) {
fprintf(stderr, "Error closing output file.\n");
- result = 1;
+ result = errno;
};
printf("Output written to %s.\n", envfilepath);
} else {
@@ -483,7 +495,6 @@ int main(int argc, char **argv)

BGENV *env_new;
BGENV *env_current;
- char *tmp;

if (auto_update) {
/* clone latest environment */
diff --git a/tools/ebgpart.c b/tools/ebgpart.c
index d0997bd..d81a630 100644
--- a/tools/ebgpart.c
+++ b/tools/ebgpart.c
@@ -43,8 +43,9 @@ static void add_block_dev(PedDevice *dev)

static char *GUID_to_str(uint8_t *g)
{
- snprintf(buffer, 37, "%02X%02X%02X%02X-%02X%02X-%02X%02X-%02X%02X-%02X%"
- "02X%02X%02X%02X%02X",
+ (void)snprintf(buffer, 37,
+ "%02X%02X%02X%02X-%02X%02X-%02X%02X-%02X%02X-"
+ "%02X%02X%02X%02X%02X%02X",
g[3], g[2], g[1], g[0], g[5], g[4], g[7], g[6], g[8], g[9],
g[10], g[11], g[12], g[13], g[14], g[15]);
return buffer;
@@ -75,7 +76,7 @@ static bool check_GPT_FAT_entry(int fd, struct EFIpartitionentry *e,
if (strcmp(GPT_PARTITION_GUID_FAT_NTFS, GUID_to_str(e->type_GUID)) !=
0 &&
strcmp(GPT_PARTITION_GUID_ESP, GUID_to_str(e->type_GUID)) != 0) {
- asprintf(&pfst->name, "not supported");
+ (void)asprintf(&pfst->name, "%s", "not supported");
return true;
}
VERBOSE(stdout, "GPT Partition #%u is FAT/NTFS.\n", i);
@@ -87,7 +88,7 @@ static bool check_GPT_FAT_entry(int fd, struct EFIpartitionentry *e,
return false;
}
/* Look if it is a FAT12 or FAT16 */
- off64_t dest = e->start_LBA * LB_SIZE + 0x36;
+ off64_t dest = (off64_t)e->start_LBA * LB_SIZE + 0x36;
if (lseek64(fd, dest, SEEK_SET) == -1) {
VERBOSE(stderr, "Error seeking FAT12/16 Id String: %s\n",
strerror(errno));
@@ -103,7 +104,7 @@ static bool check_GPT_FAT_entry(int fd, struct EFIpartitionentry *e,
if (strcmp(FAT_id, "FAT12 ") != 0 &&
strcmp(FAT_id, "FAT16 ") != 0) {
/* No FAT12/16 so read ID field for FAT32 */
- dest = e->start_LBA * LB_SIZE + 0x52;
+ dest = (off64_t)e->start_LBA * LB_SIZE + 0x52;
if (lseek64(fd, dest, SEEK_SET) == -1) {
VERBOSE(stderr, "Error seeking FAT32 Id String: %s\n",
strerror(errno));
@@ -116,11 +117,11 @@ static bool check_GPT_FAT_entry(int fd, struct EFIpartitionentry *e,
}
}
if (strcmp(FAT_id, "FAT12 ") == 0) {
- asprintf(&pfst->name, "fat12");
+ (void)asprintf(&pfst->name, "%s", "fat12");
} else if (strcmp(FAT_id, "FAT16 ") == 0) {
- asprintf(&pfst->name, "fat16");
+ (void)asprintf(&pfst->name, "%s", "fat16");
} else {
- asprintf(&pfst->name, "fat32");
+ (void)asprintf(&pfst->name, "%s", "fat32");
}
VERBOSE(stdout, "GPT Partition #%u is %s.\n", i, pfst->name);
if (lseek64(fd, curr, SEEK_SET) == -1) {
@@ -191,19 +192,19 @@ static void scanLogicalVolumes(int fd, off64_t extended_start_LBA,
PedPartition *partition, int lognum)
{
struct Masterbootrecord next_ebr;
- PedFileSystemType *pfst;
+ PedFileSystemType *pfst = NULL;

off64_t offset = extended_start_LBA + ebr->parttable[i].start_LBA;
if (extended_start_LBA == 0) {
extended_start_LBA = offset;
}
- VERBOSE(stdout, "Seeking to LBA %ld\n", offset);
+ VERBOSE(stdout, "Seeking to LBA %llu\n", (unsigned long long)offset);
off64_t res = lseek64(fd, offset * LB_SIZE, SEEK_SET);
if (res == -1) {
VERBOSE(stderr, "(%s)\n", strerror(errno));
return;
}
- VERBOSE(stdout, "Seek returned %ld\n", res);
+ VERBOSE(stdout, "Seek returned %lld\n", (signed long long)res);
if (read(fd, &next_ebr, sizeof(next_ebr)) != sizeof(next_ebr)) {
VERBOSE(stderr, "Error reading next EBR (%s)\n",
strerror(errno));
@@ -233,7 +234,7 @@ static void scanLogicalVolumes(int fd, off64_t extended_start_LBA,
if (!pfst) {
goto scl_out_of_mem;
}
- if (asprintf(&pfst->name, type_to_name(t)) == -1) {
+ if (asprintf(&pfst->name, "%s", type_to_name(t)) == -1) {
goto scl_out_of_mem;
};
partition = partition->next;
@@ -270,6 +271,7 @@ static bool check_partition_table(PedDevice *dev)
}
int numpartitions = 0;
PedPartition **list_end = &dev->part_list;
+ PedPartition *tmp = NULL;
for (int i = 0; i < 4; i++) {
if (mbr.parttable[i].partition_type == 0) {
continue;
@@ -281,10 +283,12 @@ static bool check_partition_table(PedDevice *dev)
if (t == MBR_TYPE_GPT) {
VERBOSE(stdout, "GPT header at %X\n",
mbr.parttable[i].start_LBA);
- off64_t offset = LB_SIZE * mbr.parttable[i].start_LBA;
+ off64_t offset = LB_SIZE *
+ (off64_t)mbr.parttable[i].start_LBA;
if (lseek64(fd, offset, SEEK_SET) != offset) {
VERBOSE(stderr, "Error seeking EFI Header\n.");
VERBOSE(stderr, "(%s)", strerror(errno));
+ close(fd);
return false;
}
struct EFIHeader efihdr;
@@ -301,8 +305,8 @@ static bool check_partition_table(PedDevice *dev)
efihdr.signature[6], efihdr.signature[7]);
VERBOSE(stdout, "Number of partition entries: %u\n",
efihdr.partitions);
- VERBOSE(stdout, "Partition Table @ LBA %lu\n",
- efihdr.partitiontable_LBA);
+ VERBOSE(stdout, "Partition Table @ LBA %llu\n",
+ (unsigned long long)efihdr.partitiontable_LBA);
read_GPT_entries(fd, efihdr.partitiontable_LBA,
efihdr.partitions, dev);
break;
@@ -312,7 +316,7 @@ static bool check_partition_table(PedDevice *dev)
goto cpt_out_of_mem;
}

- PedPartition *tmp = calloc(sizeof(PedPartition), 1);
+ tmp = calloc(sizeof(PedPartition), 1);
if (!tmp) {
goto cpt_out_of_mem;
}
@@ -324,7 +328,7 @@ static bool check_partition_table(PedDevice *dev)
list_end = &((*list_end)->next);

if (t == MBR_TYPE_EXTENDED || t == MBR_TYPE_EXTENDED_LBA) {
- asprintf(&pfst->name, "extended");
+ (void)asprintf(&pfst->name, "%s", "extended");
scanLogicalVolumes(fd, 0, &mbr, i, tmp, 5);
/* Could be we still have MBR entries after
* logical volumes */
@@ -332,7 +336,7 @@ static bool check_partition_table(PedDevice *dev)
list_end = &((*list_end)->next);
}
} else {
- asprintf(&pfst->name, type_to_name(t));
+ (void)asprintf(&pfst->name, "%s", type_to_name(t));
}
continue;
cpt_out_of_mem:
@@ -363,7 +367,8 @@ static int scan_devdir(unsigned int fmajor, unsigned int fminor, char *fullname,
if (!devfile) {
break;
}
- snprintf(fullname, maxlen, "%s/%s", DEVDIR, devfile->d_name);
+ (void)snprintf(fullname, maxlen, "%s/%s", DEVDIR,
+ devfile->d_name);
struct stat fstat;
if (stat(fullname, &fstat) == -1) {
VERBOSE(stderr, "stat failed on %s\n", fullname);
@@ -389,7 +394,7 @@ static int get_major_minor(char *filename, unsigned int *major, unsigned int *mi
return -1;
}
int res = fscanf(fh, "%u:%u", major, minor);
- fclose(fh);
+ (void)fclose(fh);
if (res < 2) {
VERBOSE(stderr,
"Error reading major/minor of device entry. (%s)\n",
@@ -402,7 +407,7 @@ static int get_major_minor(char *filename, unsigned int *major, unsigned int *mi
void ped_device_probe_all(void)
{
struct dirent *sysblockfile;
- char fullname[DEV_FILENAME_LEN];
+ char fullname[DEV_FILENAME_LEN+16];

DIR *sysblockdir = opendir(SYSBLOCKDIR);
if (!sysblockdir) {
@@ -420,7 +425,7 @@ void ped_device_probe_all(void)
strcmp(sysblockfile->d_name, "..") == 0) {
continue;
}
- snprintf(fullname, sizeof(fullname), "/sys/block/%s/dev",
+ (void)snprintf(fullname, sizeof(fullname), "/sys/block/%s/dev",
sysblockfile->d_name);
/* Get major and minor revision from /sys/block/sdX/dev */
unsigned int fmajor, fminor;
@@ -428,10 +433,10 @@ void ped_device_probe_all(void)
continue;
}
VERBOSE(stdout,
- "Trying device with: Major = %d, Minor = %d, (%s)\n",
+ "Trying device with: Major = %u, Minor = %u, (%s)\n",
fmajor, fminor, fullname);
/* Check if this file is really in the dev directory */
- snprintf(fullname, sizeof(fullname), "%s/%s", DEVDIR,
+ (void)snprintf(fullname, sizeof(fullname), "%s/%s", DEVDIR,
sysblockfile->d_name);
struct stat fstat;
if (stat(fullname, &fstat) == -1) {
@@ -444,8 +449,8 @@ void ped_device_probe_all(void)
}
/* This is a block device, so add it to the list*/
PedDevice *dev = calloc(sizeof(PedDevice), 1);
- asprintf(&dev->model, "N/A");
- asprintf(&dev->path, "%s", fullname);
+ (void)asprintf(&dev->model, "%s", "N/A");
+ (void)asprintf(&dev->path, "%s", fullname);
if (check_partition_table(dev)) {
add_block_dev(dev);
} else {
--
2.14.1

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

asprintf may fail to allocate memory and return -1.
Don't ignore return values and do proper error handling.

Check malloc and calloc calls to not return NULL.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---

tools/bg_setenv.c | 53 +++++++++++++++++++++++++++++++++++++++--------------
tools/ebgpart.c | 51 ++++++++++++++++++++++++++++++++++++++++-----------
2 files changed, 79 insertions(+), 25 deletions(-)

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index 31f0c61..3d5e3eb 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -65,24 +65,38 @@ struct env_action {

STAILQ_HEAD(stailhead, env_action) head = STAILQ_HEAD_INITIALIZER(head);

-static void journal_add_action(BGENV_TASK task, char *key, char *type,
- uint8_t *data, size_t datalen)
+static error_t journal_add_action(BGENV_TASK task, char *key, char *type,
+ uint8_t *data, size_t datalen)
{
struct env_action *new_action;

new_action = calloc(1, sizeof(struct env_action));

+ if (!new_action) {
+ return ENOMEM;
+ }

new_action->task = task;
if (key) {

- (void)asprintf(&(new_action->key), "%s", key);
+ if (asprintf(&(new_action->key), "%s", key) == -1) {
+ goto newaction_nomem;
+ }
}
if (type) {
- (void)asprintf(&(new_action->type), "%s", type);
+ if (asprintf(&(new_action->type), "%s", type) == -1) {
+ goto newaction_nomem;
+ }

}
if (data && datalen) {
new_action->data = (uint8_t *)malloc(datalen);

+ if (!new_action->data) {
+ goto newaction_nomem;
+ }
memcpy(new_action->data, data, datalen);
}
STAILQ_INSERT_TAIL(&head, new_action, journal);
+ return 0;
+newaction_nomem:
+ free(new_action);
+ return ENOMEM;
}

static void journal_free_action(struct env_action *action)
@@ -175,9 +189,10 @@ static char *ustate2str(uint8_t ustate)
return ustatemap[ustate];
}

-static int set_uservars(char *arg)
+static error_t set_uservars(char *arg)
{
char *key, *value;
+ error_t e;

key = strtok(arg, "=");
if (key == NULL) {
@@ -186,21 +201,22 @@ static int set_uservars(char *arg)

value = strtok(NULL, "=");
if (value == NULL) {
- journal_add_action(ENV_TASK_DEL, key, NULL, NULL, 0);
- return 0;
+ e = journal_add_action(ENV_TASK_DEL, key, NULL, NULL, 0);
+ return e;
}

- journal_add_action(ENV_TASK_SET, key, USERVAR_TYPE_DEFAULT,
- (uint8_t *)value, strlen(value) + 1);

- return 0;
+ e = journal_add_action(ENV_TASK_SET, key, USERVAR_TYPE_DEFAULT,

+ (uint8_t *)value, strlen(value) + 1);

+ return e;

}

static error_t parse_opt(int key, char *arg, struct argp_state *state)
{
struct arguments *arguments = state->input;

- int i;
+ int i, res;
char *tmp;
+ error_t e;

switch (key) {
case 'k':
@@ -263,7 +279,10 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)

ustatemap[3]);
return 1;
} else {

- (void)asprintf(&tmp, "%u", i);
+ res = asprintf(&tmp, "%u", i);
+ if (res == -1) {
+ return ENOMEM;

+ }
journal_add_action(ENV_TASK_SET, "ustate", "String",

(uint8_t *)tmp, strlen(tmp) + 1);

VERBOSE(stdout, "Ustate set to %d (%s).\n", i,

@@ -291,7 +310,10 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)

break;
case 'f':
arguments->output_to_file = true;

- (void)asprintf(&envfilepath, "%s/%s", arg, FAT_ENV_FILENAME);
+ res = asprintf(&envfilepath, "%s/%s", arg, FAT_ENV_FILENAME);
+ if (res == -1) {
+ return ENOMEM;
+ }
break;
case 'c':
VERBOSE(stdout,
@@ -318,7 +340,10 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
break;
case 'x':
/* Set user-defined variable(s) */
- set_uservars(arg);
+ e = set_uservars(arg);
+ if (e) {
+ return e;
+ }
break;
case 'V':
printf("EFI Boot Guard %s\n", EFIBOOTGUARD_VERSION);
diff --git a/tools/ebgpart.c b/tools/ebgpart.c
index d81a630..21ffc99 100644
--- a/tools/ebgpart.c
+++ b/tools/ebgpart.c
@@ -76,7 +76,9 @@ static bool check_GPT_FAT_entry(int fd, struct EFIpartitionentry *e,

if (strcmp(GPT_PARTITION_GUID_FAT_NTFS, GUID_to_str(e->type_GUID)) !=
0 &&
strcmp(GPT_PARTITION_GUID_ESP, GUID_to_str(e->type_GUID)) != 0) {

- (void)asprintf(&pfst->name, "%s", "not supported");
+ if (asprintf(&pfst->name, "%s", "not supported") == -1) {
+ goto error_asprintf;
+ }

return true;
}
VERBOSE(stdout, "GPT Partition #%u is FAT/NTFS.\n", i);

@@ -117,11 +119,17 @@ static bool check_GPT_FAT_entry(int fd, struct EFIpartitionentry *e,

}
}
if (strcmp(FAT_id, "FAT12 ") == 0) {

- (void)asprintf(&pfst->name, "%s", "fat12");
+ if (asprintf(&pfst->name, "%s", "fat12") == -1) {
+ goto error_asprintf;
+ }

} else if (strcmp(FAT_id, "FAT16 ") == 0) {

- (void)asprintf(&pfst->name, "%s", "fat16");
+ if (asprintf(&pfst->name, "%s", "fat16") == -1) {
+ goto error_asprintf;
+ }
} else {
- (void)asprintf(&pfst->name, "%s", "fat32");
+ if (asprintf(&pfst->name, "%s", "fat32") == -1) {
+ goto error_asprintf;
+ }

}
VERBOSE(stdout, "GPT Partition #%u is %s.\n", i, pfst->name);
if (lseek64(fd, curr, SEEK_SET) == -1) {

@@ -130,6 +138,10 @@ static bool check_GPT_FAT_entry(int fd, struct EFIpartitionentry *e,
return false;
}
return true;
+
+error_asprintf:
+ VERBOSE(stderr, "Error in asprintf - possibly out of memory.\n");
+ return false;
}

static void read_GPT_entries(int fd, uint64_t table_LBA, uint32_t num,
@@ -328,7 +340,9 @@ static bool check_partition_table(PedDevice *dev)

list_end = &((*list_end)->next);

if (t == MBR_TYPE_EXTENDED || t == MBR_TYPE_EXTENDED_LBA) {

- (void)asprintf(&pfst->name, "%s", "extended");
+ if (asprintf(&pfst->name, "%s", "extended") == -1) {
+ goto cpt_out_of_mem;
+ }

scanLogicalVolumes(fd, 0, &mbr, i, tmp, 5);
/* Could be we still have MBR entries after
* logical volumes */

@@ -336,10 +350,13 @@ static bool check_partition_table(PedDevice *dev)

list_end = &((*list_end)->next);
}
} else {

- (void)asprintf(&pfst->name, "%s", type_to_name(t));

+ if (asprintf(&pfst->name, "%s", type_to_name(t)) == -1) {

+ goto cpt_out_of_mem;
+ }
}
continue;
cpt_out_of_mem:
+ VERBOSE(stderr, "Out of mem while checking partition table\n.");
if (pfst) free(pfst);
if (tmp) free(tmp);
return false;
@@ -449,15 +466,27 @@ void ped_device_probe_all(void)

}
/* This is a block device, so add it to the list*/
PedDevice *dev = calloc(sizeof(PedDevice), 1);

- (void)asprintf(&dev->model, "%s", "N/A");
- (void)asprintf(&dev->path, "%s", fullname);
+ if (!dev) {
+ goto pedprobe_error;
+ }
+ if (asprintf(&dev->model, "%s", "N/A") == -1) {
+ dev->model = NULL;
+ goto pedprobe_error;
+ }
+ if (asprintf(&dev->path, "%s", fullname) == -1) {
+ dev->path = NULL;
+ goto pedprobe_error;
+ }
if (check_partition_table(dev)) {
add_block_dev(dev);
} else {
- free(dev->model);
- free(dev->path);
- free(dev);
+ goto pedprobe_error;
}
+ continue;
+pedprobe_error:
+ if (dev->model) free(dev->model);
+ if (dev->path) free(dev->path);
+ if (dev) free(dev);
} while (sysblockfile);

closedir(sysblockdir);
--
2.14.1

[Jan Kiszka]

Inconsistent indention -> visualize whitespaces. Fixing up on merge.

We either fix such an issue (lacking check of return code) or leave the
warning in until that has happened. Given that your very next patch does
that, I closing my eyes on this change... not: the next patch just
failed review. So I will remove all asprintf changes from this patch
prior to merging.

Jan

--
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

[Jan Kiszka]

On 2017-10-05 14:00, [ext] Andreas J. Reichel wrote:

The variable you are testing here against NULL are "undefined" in case
the allocation failed.

> } while (sysblockfile);
>
> closedir(sysblockdir);
>

Jan

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

asprintf may fail to allocate memory and return -1.
Don't ignore return values and do proper error handling.

Check malloc and calloc calls to not return NULL.

Simplify journal processing in bg_setenv to simplify
memory management.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---

tools/bg_setenv.c | 87 ++++++++++++++++++++++++++++++++++++-------------------
tools/ebgpart.c | 51 +++++++++++++++++++++++++-------
2 files changed, 98 insertions(+), 40 deletions(-)

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index 58687f4..4b461a5 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -65,31 +65,54 @@ struct env_action {

STAILQ_HEAD(stailhead, env_action) head = STAILQ_HEAD_INITIALIZER(head);

-static void journal_add_action(BGENV_TASK task, char *key, char *type,
- uint8_t *data, size_t datalen)

+static void journal_free_action(struct env_action *action)
+{
+ if (!action)
+ return;
+ if (action->data)
+ free(action->data);
+ if (action->type)
+ free(action->type);
+ if (action->key)
+ free(action->key);
+ free(action);
+}
+

+static error_t journal_add_action(BGENV_TASK task, char *key, char *type,
+ uint8_t *data, size_t datalen)
{
struct env_action *new_action;

new_action = calloc(1, sizeof(struct env_action));
+ if (!new_action) {
+ return ENOMEM;
+ }
new_action->task = task;
if (key) {

- asprintf(&(new_action->key), "%s", key);

+ if (asprintf(&(new_action->key), "%s", key) == -1) {

+ new_action->key = NULL;

+ goto newaction_nomem;
+ }
}
if (type) {

- asprintf(&(new_action->type), "%s", type);

+ if (asprintf(&(new_action->type), "%s", type) == -1) {

+ new_action->type = NULL;

+ goto newaction_nomem;
+ }
}
if (data && datalen) {
new_action->data = (uint8_t *)malloc(datalen);
+ if (!new_action->data) {

+ new_action->data = NULL;

+ goto newaction_nomem;
+ }
memcpy(new_action->data, data, datalen);
}
STAILQ_INSERT_TAIL(&head, new_action, journal);

-}
-
-static void journal_free_action(struct env_action *action)
-{
- if (action->data) free(action->data);
- if (action->type) free(action->type);
- if (action->key) free(action->key);
+ return 0;
+newaction_nomem:
+ journal_free_action(new_action);
+ return ENOMEM;
}

static void journal_process_action(BGENV *env, struct env_action *action)

@@ -175,9 +198,10 @@ static char *ustate2str(uint8_t ustate)

return ustatemap[ustate];
}

-static int set_uservars(char *arg)
+static error_t set_uservars(char *arg)
{
char *key, *value;
+ error_t e;

key = strtok(arg, "=");
if (key == NULL) {

@@ -186,21 +210,22 @@ static int set_uservars(char *arg)

value = strtok(NULL, "=");
if (value == NULL) {
- journal_add_action(ENV_TASK_DEL, key, NULL, NULL, 0);
- return 0;
+ e = journal_add_action(ENV_TASK_DEL, key, NULL, NULL, 0);
+ return e;
}

- journal_add_action(ENV_TASK_SET, key, USERVAR_TYPE_DEFAULT,
- (uint8_t *)value, strlen(value) + 1);

- return 0;
+ e = journal_add_action(ENV_TASK_SET, key, USERVAR_TYPE_DEFAULT,
+ (uint8_t *)value, strlen(value) + 1);
+ return e;
}

static error_t parse_opt(int key, char *arg, struct argp_state *state)
{
struct arguments *arguments = state->input;
- int i;
+ int i, res;
char *tmp;
+ error_t e;

switch (key) {
case 'k':

@@ -263,7 +288,10 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)

ustatemap[3]);
return 1;
} else {

- asprintf(&tmp, "%u", i);

+ res = asprintf(&tmp, "%u", i);
+ if (res == -1) {
+ return ENOMEM;
+ }
journal_add_action(ENV_TASK_SET, "ustate", "String",
(uint8_t *)tmp, strlen(tmp) + 1);
VERBOSE(stdout, "Ustate set to %d (%s).\n", i,

@@ -291,7 +319,10 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)

break;
case 'f':
arguments->output_to_file = true;

- asprintf(&envfilepath, "%s/%s", arg, FAT_ENV_FILENAME);

+ res = asprintf(&envfilepath, "%s/%s", arg, FAT_ENV_FILENAME);
+ if (res == -1) {
+ return ENOMEM;
+ }
break;
case 'c':
VERBOSE(stdout,

@@ -318,7 +349,10 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)

break;
case 'x':
/* Set user-defined variable(s) */
- set_uservars(arg);
+ e = set_uservars(arg);
+ if (e) {
+ return e;
+ }
break;
case 'V':
printf("EFI Boot Guard %s\n", EFIBOOTGUARD_VERSION);

@@ -374,21 +408,16 @@ static void update_environment(BGENV *env)

printf("Processing journal...\n");

- STAILQ_FOREACH(action, &head, journal)
- {

+ while (!STAILQ_EMPTY(&head)) {
+ action = STAILQ_FIRST(&head);

journal_process_action(env, action);
+ STAILQ_REMOVE_HEAD(&head, journal);
journal_free_action(action);
- STAILQ_REMOVE(&head, action, env_action, journal);

}

env->data->crc32 = crc32(0, (const Bytef *)env->data,
sizeof(BG_ENVDATA) - sizeof(env->data->crc32));

- while (!STAILQ_EMPTY(&head)) {
- action = STAILQ_FIRST(&head);
- STAILQ_REMOVE_HEAD(&head, journal);
- free(action);
- }
}

static void dump_envs(void)
diff --git a/tools/ebgpart.c b/tools/ebgpart.c
index ed8a0bb..d5b5de3 100644

--- a/tools/ebgpart.c
+++ b/tools/ebgpart.c
@@ -76,7 +76,9 @@ static bool check_GPT_FAT_entry(int fd, struct EFIpartitionentry *e,
if (strcmp(GPT_PARTITION_GUID_FAT_NTFS, GUID_to_str(e->type_GUID)) !=
0 &&
strcmp(GPT_PARTITION_GUID_ESP, GUID_to_str(e->type_GUID)) != 0) {

- asprintf(&pfst->name, "%s", "not supported");

+ if (asprintf(&pfst->name, "%s", "not supported") == -1) {
+ goto error_asprintf;
+ }
return true;
}
VERBOSE(stdout, "GPT Partition #%u is FAT/NTFS.\n", i);
@@ -117,11 +119,17 @@ static bool check_GPT_FAT_entry(int fd, struct EFIpartitionentry *e,
}
}
if (strcmp(FAT_id, "FAT12 ") == 0) {

- asprintf(&pfst->name, "%s", "fat12");

+ if (asprintf(&pfst->name, "%s", "fat12") == -1) {
+ goto error_asprintf;
+ }
} else if (strcmp(FAT_id, "FAT16 ") == 0) {

- asprintf(&pfst->name, "%s", "fat16");

+ if (asprintf(&pfst->name, "%s", "fat16") == -1) {
+ goto error_asprintf;
+ }
} else {

- asprintf(&pfst->name, "%s", "fat32");

+ if (asprintf(&pfst->name, "%s", "fat32") == -1) {
+ goto error_asprintf;
+ }
}
VERBOSE(stdout, "GPT Partition #%u is %s.\n", i, pfst->name);
if (lseek64(fd, curr, SEEK_SET) == -1) {
@@ -130,6 +138,10 @@ static bool check_GPT_FAT_entry(int fd, struct EFIpartitionentry *e,
return false;
}
return true;
+
+error_asprintf:
+ VERBOSE(stderr, "Error in asprintf - possibly out of memory.\n");
+ return false;
}

static void read_GPT_entries(int fd, uint64_t table_LBA, uint32_t num,

@@ -329,7 +341,9 @@ static bool check_partition_table(PedDevice *dev)

list_end = &((*list_end)->next);

if (t == MBR_TYPE_EXTENDED || t == MBR_TYPE_EXTENDED_LBA) {

- asprintf(&pfst->name, "%s", "extended");

+ if (asprintf(&pfst->name, "%s", "extended") == -1) {
+ goto cpt_out_of_mem;
+ }
scanLogicalVolumes(fd, 0, &mbr, i, tmp, 5);
/* Could be we still have MBR entries after
* logical volumes */

@@ -337,11 +351,14 @@ static bool check_partition_table(PedDevice *dev)

list_end = &((*list_end)->next);
}
} else {

- asprintf(&pfst->name, "%s", type_to_name(t));

+ if (asprintf(&pfst->name, "%s", type_to_name(t)) == -1) {
+ goto cpt_out_of_mem;
+ }
}
continue;
cpt_out_of_mem:

close(fd);

+ VERBOSE(stderr, "Out of mem while checking partition table\n.");
if (pfst) free(pfst);
if (tmp) free(tmp);
return false;

@@ -451,15 +468,27 @@ void ped_device_probe_all(void)

}
/* This is a block device, so add it to the list*/
PedDevice *dev = calloc(sizeof(PedDevice), 1);

- asprintf(&dev->model, "%s", "N/A");
- asprintf(&dev->path, "%s", fullname);
+ if (!dev) {
+ continue;

+ }
+ if (asprintf(&dev->model, "%s", "N/A") == -1) {
+ dev->model = NULL;
+ goto pedprobe_error;
+ }
+ if (asprintf(&dev->path, "%s", fullname) == -1) {
+ dev->path = NULL;
+ goto pedprobe_error;
+ }
if (check_partition_table(dev)) {
add_block_dev(dev);

- } else {
+ continue;
+ }

+pedprobe_error:
+ if (dev->model)
free(dev->model);
+ if (dev->path)
free(dev->path);

- free(dev);
- }
+ free(dev);
} while (sysblockfile);

closedir(sysblockdir);
--
2.14.2

[Jan Kiszka]

Just do free(NULL), that's legal. And simpler.

> +}
> +
> +static error_t journal_add_action(BGENV_TASK task, char *key, char *type,
> + uint8_t *data, size_t datalen)

Make sure to check all callers evaluate the return code. At least one
case seems to be missed, see below.

Grant a blank line, for the sake of readability.

> +newaction_nomem:
> + journal_free_action(new_action);
> + return ENOMEM;
> }
>
> static void journal_process_action(BGENV *env, struct env_action *action)
> @@ -175,9 +198,10 @@ static char *ustate2str(uint8_t ustate)
> return ustatemap[ustate];
> }
>
> -static int set_uservars(char *arg)
> +static error_t set_uservars(char *arg)
> {
> char *key, *value;
> + error_t e;
>
> key = strtok(arg, "=");
> if (key == NULL) {
> @@ -186,21 +210,22 @@ static int set_uservars(char *arg)
>
> value = strtok(NULL, "=");
> if (value == NULL) {
> - journal_add_action(ENV_TASK_DEL, key, NULL, NULL, 0);
> - return 0;
> + e = journal_add_action(ENV_TASK_DEL, key, NULL, NULL, 0);
> + return e;

Why not just "return journal_add_action()"?

> }
>
> - journal_add_action(ENV_TASK_SET, key, USERVAR_TYPE_DEFAULT,
> - (uint8_t *)value, strlen(value) + 1);
>
> - return 0;
> + e = journal_add_action(ENV_TASK_SET, key, USERVAR_TYPE_DEFAULT,
> + (uint8_t *)value, strlen(value) + 1);
> + return e;

Same here.

> }
>
> static error_t parse_opt(int key, char *arg, struct argp_state *state)
> {
> struct arguments *arguments = state->input;
> - int i;
> + int i, res;
> char *tmp;
> + error_t e;
>
> switch (key) {
> case 'k':
> @@ -263,7 +288,10 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
> ustatemap[3]);
> return 1;
> } else {
> - asprintf(&tmp, "%u", i);
> + res = asprintf(&tmp, "%u", i);
> + if (res == -1) {
> + return ENOMEM;
> + }
> journal_add_action(ENV_TASK_SET, "ustate", "String",
> (uint8_t *)tmp, strlen(tmp) + 1);

journal_add_action cannot fail here? Then we need a comment.

Getting closer :)

Jan

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

asprintf may fail to allocate memory and return -1.
Don't ignore return values and do proper error handling.

Check malloc and calloc calls to not return NULL.

Simplify journal processing in bg_setenv to simplify
memory management.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---

tools/bg_setenv.c | 112 +++++++++++++++++++++++++++++++++---------------------
tools/ebgpart.c | 51 +++++++++++++++++++------
2 files changed, 108 insertions(+), 55 deletions(-)

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index 58687f4..4878226 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -65,31 +65,52 @@ struct env_action {

STAILQ_HEAD(stailhead, env_action) head = STAILQ_HEAD_INITIALIZER(head);

-static void journal_add_action(BGENV_TASK task, char *key, char *type,
- uint8_t *data, size_t datalen)
+static void journal_free_action(struct env_action *action)
+{
+ if (!action)
+ return;

+ free(action->data);
+ free(action->type);

+ free(action->key);
+ free(action);

+}
+
+static error_t journal_add_action(BGENV_TASK task, char *key, char *type,
+ uint8_t *data, size_t datalen)

+ return 0;

-static void journal_free_action(struct env_action *action)
-{
- if (action->data) free(action->data);
- if (action->type) free(action->type);
- if (action->key) free(action->key);

+newaction_nomem:
+ journal_free_action(new_action);
+ return ENOMEM;
}

static void journal_process_action(BGENV *env, struct env_action *action)

@@ -175,7 +196,7 @@ static char *ustate2str(uint8_t ustate)

return ustatemap[ustate];
}

-static int set_uservars(char *arg)
+static error_t set_uservars(char *arg)
{
char *key, *value;

@@ -186,21 +207,18 @@ static int set_uservars(char *arg)

value = strtok(NULL, "=");
if (value == NULL) {
- journal_add_action(ENV_TASK_DEL, key, NULL, NULL, 0);
- return 0;

+ return journal_add_action(ENV_TASK_DEL, key, NULL, NULL, 0);
}
-

- journal_add_action(ENV_TASK_SET, key, USERVAR_TYPE_DEFAULT,
- (uint8_t *)value, strlen(value) + 1);
-

- return 0;
+ return journal_add_action(ENV_TASK_SET, key, USERVAR_TYPE_DEFAULT,

+ (uint8_t *)value, strlen(value) + 1);
}

static error_t parse_opt(int key, char *arg, struct argp_state *state)
{
struct arguments *arguments = state->input;
- int i;
+ int i, res;
char *tmp;

+ error_t e = 0;

switch (key) {
case 'k':
@@ -211,8 +229,8 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)

ENV_STRING_LENGTH);
return 1;
}
- journal_add_action(ENV_TASK_SET, "kernelfile", "String",

- (uint8_t *)arg, strlen(arg) + 1);
+ e = journal_add_action(ENV_TASK_SET, "kernelfile", "String",

+ (uint8_t *)arg, strlen(arg) + 1);
break;
case 'a':
if (strlen(arg) > ENV_STRING_LENGTH) {

@@ -222,8 +240,8 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)

ENV_STRING_LENGTH);
return 1;
}
- journal_add_action(ENV_TASK_SET, "kernelparams", "String",

- (uint8_t *)arg, strlen(arg) + 1);
+ e = journal_add_action(ENV_TASK_SET, "kernelparams", "String",

+ (uint8_t *)arg, strlen(arg) + 1);
break;
case 'p':
i = strtol(arg, &tmp, 10);

@@ -263,9 +281,12 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)

ustatemap[3]);
return 1;
} else {
- asprintf(&tmp, "%u", i);

- journal_add_action(ENV_TASK_SET, "ustate", "String",
- (uint8_t *)tmp, strlen(tmp) + 1);

+ res = asprintf(&tmp, "%u", i);
+ if (res == -1) {
+ return ENOMEM;
+ }

+ e = journal_add_action(ENV_TASK_SET, "ustate", "String",
+ (uint8_t *)tmp, strlen(tmp) + 1);

VERBOSE(stdout, "Ustate set to %d (%s).\n", i,

ustate2str(i));
}
@@ -273,17 +294,18 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)

case 'r':
i = atoi(arg);
VERBOSE(stdout, "Revision is set to %d.\n", i);
- journal_add_action(ENV_TASK_SET, "revision", "String",

- (uint8_t *)arg, strlen(arg) + 1);
+ e = journal_add_action(ENV_TASK_SET, "revision", "String",

+ (uint8_t *)arg, strlen(arg) + 1);
break;
case 'w':
i = atoi(arg);

if (i != 0) {

VERBOSE(stdout,
"Setting watchdog timeout to %d seconds.\n", i);

- journal_add_action(ENV_TASK_SET, "watchdog_timeout_sec",
- "String", (uint8_t *)arg,
- strlen(arg) + 1);
+ e = journal_add_action(ENV_TASK_SET,
+ "watchdog_timeout_sec",

+ "String", (uint8_t *)arg,
+ strlen(arg) + 1);
} else {
fprintf(stderr, "Watchdog timeout must be non-zero.\n");
return 1;

@@ -291,14 +313,17 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)

break;
case 'f':
arguments->output_to_file = true;
- asprintf(&envfilepath, "%s/%s", arg, FAT_ENV_FILENAME);
+ res = asprintf(&envfilepath, "%s/%s", arg, FAT_ENV_FILENAME);
+ if (res == -1) {
+ return ENOMEM;
+ }
break;
case 'c':
VERBOSE(stdout,

"Confirming environment to work. Removing boot-once "
"and testing flag.\n");

- journal_add_action(ENV_TASK_SET, "ustate", "String",
- (uint8_t *)"0", 2);
+ e = journal_add_action(ENV_TASK_SET, "ustate", "String",

+ (uint8_t *)"0", 2);
break;
case 'u':
if (part_specified) {

@@ -318,7 +343,7 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)

break;
case 'x':
/* Set user-defined variable(s) */
- set_uservars(arg);
+ e = set_uservars(arg);

break;
case 'V':
printf("EFI Boot Guard %s\n", EFIBOOTGUARD_VERSION);

@@ -331,7 +356,11 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
default:
return ARGP_ERR_UNKNOWN;
}
- return 0;
+
+ if (e) {
+ fprintf(stderr, "Error creating journal: %s\n", strerror(e));
+ }
+ return e;
}

static void dump_uservars(uint8_t *udata)
@@ -374,21 +403,16 @@ static void update_environment(BGENV *env)

--
2.14.2

[Jan Kiszka]

Still some redundant if (NULL), I will remove them on merge.

> - free(dev);
> - }
> + free(dev);
> } while (sysblockfile);
>
> closedir(sysblockdir);
>

Thanks,