From 8198c3e42b0614b6bd1db6f38813b842c8577304 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 4 Oct 2019 16:17:27 +0200 Subject: pstore: rework memory handling for dmesg Semmle Security Reports report: > The problem occurs on the way realloc is being used. When a size > bigger than the chunk that wants to be reallocated is passed, realloc > try to malloc a bigger size, however in the case that malloc fails > (for example, by forcing a big allocation) realloc will return NULL. > > According to the man page: > "The realloc() function returns a pointer to the newly allocated > memory, which is suitably aligned for any built-in type and may be > different from ptr, or NULL if the request fails. If size was > equal to 0, either NULL or a pointer suitable to be passed to free() > is returned. If realloc() fails, the original block is left > untouched; it is not freed or moved." > > The problem occurs when the memory ptr passed to the first argument of > realloc is the same as the one used for the result, for example in > this case: > > dmesg = realloc(dmesg, dmesg_size + strlen(pe->dirent.d_name) + > strlen(":\n") + pe->content_size + 1); > > https://lgtm.com/projects/g/systemd/systemd/snapshot/f8bcb81955f9e93a4787627e28f43fffb2a84836/files/src/pstore/pstore.c?sort=name&dir=A > SC&mode=heatmap#L300 > > If the malloc inside that realloc fails, then the original memory > chunk will never be free but since realloc will return NULL, the > pointer to that memory chunk will be lost and a memory leak will > occur. > > In case you are curious, this is the query we used to find this problem: > https://lgtm.com/query/8650323308193591473/ Let's use a more standard pattern: allocate memory using greedy_realloc, and instead of freeing it when we wrote out a chunk, let's just move the cursor back to the beginning and reuse the memory we allocated previously. If we fail to allocate the memory for dmesg contents, don't write the dmesg entry, but let's still process the files to move them out of pstore. --- src/pstore/pstore.c | 43 ++++++++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 17 deletions(-) (limited to 'src/pstore') diff --git a/src/pstore/pstore.c b/src/pstore/pstore.c index 9861758700..e4618aac0e 100644 --- a/src/pstore/pstore.c +++ b/src/pstore/pstore.c @@ -179,9 +179,11 @@ static int write_dmesg(const char *dmesg, size_t size, const char *id) { ssize_t wr; int r; - if (isempty(dmesg) || size == 0) + if (size == 0) return 0; + assert(dmesg); + ofd_path = path_join(arg_archivedir, id, "dmesg.txt"); if (!ofd_path) return log_oom(); @@ -205,7 +207,8 @@ static int write_dmesg(const char *dmesg, size_t size, const char *id) { static void process_dmesg_files(PStoreList *list) { /* Move files, reconstruct dmesg.txt */ _cleanup_free_ char *dmesg = NULL, *dmesg_id = NULL; - size_t dmesg_size = 0; + size_t dmesg_size = 0, dmesg_allocated = 0; + bool dmesg_bad = false; PStoreEntry *pe; /* Handle each dmesg file: files processed in reverse @@ -282,33 +285,39 @@ static void process_dmesg_files(PStoreList *list) { /* Now move file from pstore to archive storage */ move_file(pe, pe_id); + if (dmesg_bad) + continue; + /* If the current record id is NOT the same as the * previous record id, then start a new dmesg.txt file */ - if (!pe_id || !dmesg_id || !streq(pe_id, dmesg_id)) { + if (!streq_ptr(pe_id, dmesg_id)) { /* Encountered a new dmesg group, close out old one, open new one */ - if (dmesg) { - (void) write_dmesg(dmesg, dmesg_size, dmesg_id); - dmesg = mfree(dmesg); - dmesg_size = 0; - } + (void) write_dmesg(dmesg, dmesg_size, dmesg_id); + dmesg_size = 0; /* now point dmesg_id to storage of pe_id */ free_and_replace(dmesg_id, pe_id); } - /* Reconstruction of dmesg is done as a useful courtesy, do not log errors */ - dmesg = realloc(dmesg, dmesg_size + strlen(pe->dirent.d_name) + strlen(":\n") + pe->content_size + 1); - if (dmesg) { - dmesg_size += sprintf(&dmesg[dmesg_size], "%s:\n", pe->dirent.d_name); - if (pe->content) { - memcpy(&dmesg[dmesg_size], pe->content, pe->content_size); - dmesg_size += pe->content_size; - } + /* Reconstruction of dmesg is done as a useful courtesy: do not fail, but don't write garbled + * output either. */ + size_t needed = strlen(pe->dirent.d_name) + strlen(":\n") + pe->content_size + 1; + if (!GREEDY_REALLOC(dmesg, dmesg_allocated, dmesg_size + needed)) { + log_warning_errno(ENOMEM, "Failed to write dmesg file: %m"); + dmesg_bad = true; + continue; + } + + dmesg_size += sprintf(dmesg + dmesg_size, "%s:\n", pe->dirent.d_name); + if (pe->content) { + memcpy(dmesg + dmesg_size, pe->content, pe->content_size); + dmesg_size += pe->content_size; } pe_id = mfree(pe_id); } - if (dmesg) + + if (!dmesg_bad) (void) write_dmesg(dmesg, dmesg_size, dmesg_id); } -- cgit v1.2.1