From 8249fa8dee2f9a4d1df6ccf1f89596daf46af7fb Mon Sep 17 00:00:00 2001 From: "Bobby R. Bruce" Date: Sat, 23 Mar 2024 14:22:12 -0700 Subject: [PATCH 1/2] base: Fix 'doGzipLoad' str manipulation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When running `scons build/ALL/gem5.opt --with-ubsan`, with GCC, the following error was returned: ``` [ CXX] src/base/loader/image_file_data.cc -> ALL/base/loader/image_file_data.o In file included from /usr/include/string.h:535, from /usr/include/c++/11/cstring:42, from src/base/cprintf_formats.hh:33, from src/base/cprintf.hh:38, from src/base/logging.hh:49, from src/base/loader/image_file_data.cc:40: In function ‘char* strcpy(char*, const char*)’, inlined from ‘int gem5::loader::doGzipLoad(int)’ at src/base/loader/image_file_data.cc:70:11, inlined from ‘gem5::loader::ImageFileData::ImageFileData(const string&)’ atsrc/base/loader/image_file_data.cc:116:24: /usr/include/x86_64-linux-gnu/bits/string_fortified.h:79:33: error: ‘void* __builtin_memcpy(void*, const void*, long unsigned int)’ offset [0, 19] is out of the bounds [0, 0] [-Werror=array-bounds] 79 | return __builtin___strcpy_chk (__dest, __src, __glibc_objsize (__dest)); | ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1plus: all warnings being treated as errors scons: *** [build/ALL/base/loader/image_file_data.o] Error 1 scons: building terminated because of errors. ``` I do not know the exact issue but using strcpy in this way (i.e. `strcpy(char_pointer + offset, string)`) appears to trigger this error with the undefined behavior sanitizer. The fix in this patch replaces this with `strcat`. Change-Id: I1a0c50c9022adc841e175aad0fe2247bfcb29d71 --- src/base/loader/image_file_data.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/base/loader/image_file_data.cc b/src/base/loader/image_file_data.cc index 525d577936..09db89f3e3 100644 --- a/src/base/loader/image_file_data.cc +++ b/src/base/loader/image_file_data.cc @@ -67,7 +67,7 @@ doGzipLoad(int fd) size_t tmp_len = strlen(P_tmpdir); char *tmpnam = (char*) malloc(tmp_len + 20); strcpy(tmpnam, P_tmpdir); - strcpy(tmpnam+tmp_len, "/gem5-gz-obj-XXXXXX"); // 19 chars + strcat(tmpnam, "/gem5-gz-obj-XXXXXX"); // concat 19 chars fd = mkstemp(tmpnam); // repurposing fd variable for output if (fd < 0) { free(tmpnam); From 55c58da504235d60edba990e5fa09f02f5e76ce5 Mon Sep 17 00:00:00 2001 From: "Bobby R. Bruce" Date: Wed, 27 Mar 2024 12:21:42 -0700 Subject: [PATCH 2/2] base: Convert doGzipLoad to use std::string instead of *char Change-Id: I28c9bf7853267686402b43be00f857914770f7a7 --- src/base/loader/image_file_data.cc | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/base/loader/image_file_data.cc b/src/base/loader/image_file_data.cc index 09db89f3e3..b402b3bbb8 100644 --- a/src/base/loader/image_file_data.cc +++ b/src/base/loader/image_file_data.cc @@ -64,13 +64,10 @@ doGzipLoad(int fd) return -1; } - size_t tmp_len = strlen(P_tmpdir); - char *tmpnam = (char*) malloc(tmp_len + 20); - strcpy(tmpnam, P_tmpdir); - strcat(tmpnam, "/gem5-gz-obj-XXXXXX"); // concat 19 chars + std::string tmpnam_str = std::string(P_tmpdir) + "/gem5-gz-obj-XXXXXX"; + char *tmpnam = const_cast(tmpnam_str.c_str()); fd = mkstemp(tmpnam); // repurposing fd variable for output if (fd < 0) { - free(tmpnam); gzclose(fdz); return fd; } @@ -78,8 +75,6 @@ doGzipLoad(int fd) if (unlink(tmpnam) != 0) warn("couldn't remove temporary file %s\n", tmpnam); - free(tmpnam); - auto buf = new uint8_t[blk_sz]; int r; // size of (r)emaining uncopied data in (buf)fer while ((r = gzread(fdz, buf, blk_sz)) > 0) {