From: Alan Modra Date: Sun, 12 Jan 2020 21:42:18 +0000 (+1030) Subject: Memory leaks and ineffective bounds checking in wasm_scan X-Git-Url: http://git.efficios.com/?p=deliverable%2Fbinutils-gdb.git;a=commitdiff_plain;h=0c0adcc52478ebb707ed780173e18262df6eab7e Memory leaks and ineffective bounds checking in wasm_scan It's always a bad idea to perform arithmetic on an unknown value read from an object file before comparing against bounds. Code like the following attempting to bounds check "len", a 64-bit value, isn't effective because the pointer arithmetic ignores the high 32 bits when compiled for a 32-bit host. READ_LEB128 (len, p, end); if (p + len < p || p + len > end) goto error_return; Instead, perform any arithmetic on known values where we don't need to worry about overflows: READ_LEB128 (len, p, end); if (len > (size_t) (end - p)) goto error_return; I'll note that this check does do things the right way: READ_LEB128 (symcount, p, end); /* Sanity check: each symbol has at least two bytes. */ if (symcount > payload_size / 2) return FALSE; "symcount * 2 > payload_size" would be wrong since the multiply could overflow. * wasm-module.c (wasm_scan_name_function_section): Formatting. Delete asect name check. Move asect NULL check to wasm_object_p. Correct bounds check of sizes against end. Replace uses of bfd_zalloc with bfd_alloc, zeroing only necessary bytes. Use just one bfd_release. (wasm_scan): Don't use malloc/strdup for section names, bfd_alloc instead. Simplify code prefixing section name. Formatting. Don't attempt to free memory here.. (wasm_object_p): ..do so here. Formatting. --- diff --git a/bfd/ChangeLog b/bfd/ChangeLog index 00d6cd810b..70944d3c80 100644 --- a/bfd/ChangeLog +++ b/bfd/ChangeLog @@ -1,3 +1,15 @@ +2020-01-13 Alan Modra + + * wasm-module.c (wasm_scan_name_function_section): Formatting. + Delete asect name check. Move asect NULL check to wasm_object_p. + Correct bounds check of sizes against end. Replace uses of + bfd_zalloc with bfd_alloc, zeroing only necessary bytes. Use + just one bfd_release. + (wasm_scan): Don't use malloc/strdup for section names, + bfd_alloc instead. Simplify code prefixing section name. + Formatting. Don't attempt to free memory here.. + (wasm_object_p): ..do so here. + 2020-01-10 Szabolcs Nagy PR ld/22269 diff --git a/bfd/wasm-module.c b/bfd/wasm-module.c index 06a964c12d..e62f842e12 100644 --- a/bfd/wasm-module.c +++ b/bfd/wasm-module.c @@ -246,16 +246,10 @@ wasm_scan_name_function_section (bfd *abfd, sec_ptr asect) asymbol *symbols = NULL; sec_ptr space_function_index; - if (! asect) - return FALSE; - - if (strcmp (asect->name, WASM_NAME_SECTION) != 0) - return FALSE; - p = asect->contents; end = asect->contents + asect->size; - if (! p) + if (!p) return FALSE; while (p < end) @@ -272,7 +266,7 @@ wasm_scan_name_function_section (bfd *abfd, sec_ptr asect) READ_LEB128 (payload_size, p, end); - if (p > p + payload_size) + if (payload_size > (size_t) (end - p)) return FALSE; p += payload_size; @@ -283,10 +277,7 @@ wasm_scan_name_function_section (bfd *abfd, sec_ptr asect) READ_LEB128 (payload_size, p, end); - if (p > p + payload_size) - return FALSE; - - if (p + payload_size > end) + if (payload_size > (size_t) (end - p)) return FALSE; end = p + payload_size; @@ -294,22 +285,24 @@ wasm_scan_name_function_section (bfd *abfd, sec_ptr asect) READ_LEB128 (symcount, p, end); /* Sanity check: each symbol has at least two bytes. */ - if (symcount > payload_size/2) + if (symcount > payload_size / 2) return FALSE; tdata->symcount = symcount; - space_function_index = bfd_make_section_with_flags - (abfd, WASM_SECTION_FUNCTION_INDEX, SEC_READONLY | SEC_CODE); + space_function_index + = bfd_make_section_with_flags (abfd, WASM_SECTION_FUNCTION_INDEX, + SEC_READONLY | SEC_CODE); - if (! space_function_index) - space_function_index = bfd_get_section_by_name (abfd, WASM_SECTION_FUNCTION_INDEX); + if (!space_function_index) + space_function_index + = bfd_get_section_by_name (abfd, WASM_SECTION_FUNCTION_INDEX); - if (! space_function_index) + if (!space_function_index) return FALSE; - symbols = bfd_zalloc (abfd, tdata->symcount * sizeof (asymbol)); - if (! symbols) + symbols = bfd_alloc2 (abfd, tdata->symcount, sizeof (asymbol)); + if (!symbols) return FALSE; for (symcount = 0; p < end && symcount < tdata->symcount; symcount++) @@ -322,14 +315,15 @@ wasm_scan_name_function_section (bfd *abfd, sec_ptr asect) READ_LEB128 (idx, p, end); READ_LEB128 (len, p, end); - if (p + len < p || p + len > end) + if (len > (size_t) (end - p)) goto error_return; - name = bfd_zalloc (abfd, len + 1); - if (! name) + name = bfd_alloc (abfd, len + 1); + if (!name) goto error_return; memcpy (name, p, len); + name[len] = 0; p += len; sym = &symbols[symcount]; @@ -350,8 +344,6 @@ wasm_scan_name_function_section (bfd *abfd, sec_ptr asect) return TRUE; error_return: - while (symcount) - bfd_release (abfd, (void *)symbols[--symcount].name); bfd_release (abfd, symbols); return FALSE; } @@ -386,13 +378,12 @@ wasm_scan (bfd *abfd) bfd_vma vma = 0x80000000; int section_code; unsigned int bytes_read; - char *name = NULL; asection *bfdsec; if (bfd_seek (abfd, (file_ptr) 0, SEEK_SET) != 0) goto error_return; - if (! wasm_read_header (abfd, &error)) + if (!wasm_read_header (abfd, &error)) goto error_return; while ((section_code = wasm_read_byte (abfd, &error)) != EOF) @@ -401,14 +392,13 @@ wasm_scan (bfd *abfd) { const char *sname = wasm_section_code_to_name (section_code); - if (! sname) + if (!sname) goto error_return; - name = strdup (sname); - bfdsec = bfd_make_section_anyway_with_flags (abfd, name, SEC_HAS_CONTENTS); + bfdsec = bfd_make_section_anyway_with_flags (abfd, sname, + SEC_HAS_CONTENTS); if (bfdsec == NULL) goto error_return; - name = NULL; bfdsec->vma = vma; bfdsec->lma = vma; @@ -423,9 +413,9 @@ wasm_scan (bfd *abfd) bfd_vma payload_len; file_ptr section_start; bfd_vma namelen; + char *name; char *prefix = WASM_SECTION_PREFIX; - char *p; - int ret; + size_t prefixlen = strlen (prefix); payload_len = wasm_read_leb128 (abfd, &error, &bytes_read, FALSE); if (error) @@ -434,21 +424,18 @@ wasm_scan (bfd *abfd) namelen = wasm_read_leb128 (abfd, &error, &bytes_read, FALSE); if (error || namelen > payload_len) goto error_return; - name = bfd_zmalloc (namelen + strlen (prefix) + 1); - if (! name) - goto error_return; - p = name; - ret = sprintf (p, "%s", prefix); - if (ret < 0 || (bfd_vma) ret != strlen (prefix)) + name = bfd_alloc (abfd, namelen + prefixlen + 1); + if (!name) goto error_return; - p += ret; - if (bfd_bread (p, namelen, abfd) != namelen) + memcpy (name, prefix, prefixlen); + if (bfd_bread (name + prefixlen, namelen, abfd) != namelen) goto error_return; + name[prefixlen + namelen] = 0; - bfdsec = bfd_make_section_anyway_with_flags (abfd, name, SEC_HAS_CONTENTS); + bfdsec = bfd_make_section_anyway_with_flags (abfd, name, + SEC_HAS_CONTENTS); if (bfdsec == NULL) goto error_return; - name = NULL; bfdsec->vma = vma; bfdsec->lma = vma; @@ -459,8 +446,8 @@ wasm_scan (bfd *abfd) if (bfdsec->size != 0) { - bfdsec->contents = bfd_zalloc (abfd, bfdsec->size); - if (! bfdsec->contents) + bfdsec->contents = bfd_alloc (abfd, bfdsec->size); + if (!bfdsec->contents) goto error_return; if (bfd_bread (bfdsec->contents, bfdsec->size, abfd) != bfdsec->size) @@ -478,12 +465,6 @@ wasm_scan (bfd *abfd) return TRUE; error_return: - if (name) - free (name); - - for (bfdsec = abfd->sections; bfdsec; bfdsec = bfdsec->next) - free ((void *) bfdsec->name); - return FALSE; } @@ -750,23 +731,30 @@ static const bfd_target * wasm_object_p (bfd *abfd) { bfd_boolean error; + asection *s; if (bfd_seek (abfd, (file_ptr) 0, SEEK_SET) != 0) return NULL; - if (! wasm_read_header (abfd, &error)) + if (!wasm_read_header (abfd, &error)) { bfd_set_error (bfd_error_wrong_format); return NULL; } - if (! wasm_mkobject (abfd) || ! wasm_scan (abfd)) + if (!wasm_mkobject (abfd)) return NULL; - if (! bfd_default_set_arch_mach (abfd, bfd_arch_wasm32, 0)) - return NULL; + if (!wasm_scan (abfd) + || !bfd_default_set_arch_mach (abfd, bfd_arch_wasm32, 0)) + { + bfd_release (abfd, abfd->tdata.any); + abfd->tdata.any = NULL; + return NULL; + } - if (wasm_scan_name_function_section (abfd, bfd_get_section_by_name (abfd, WASM_NAME_SECTION))) + s = bfd_get_section_by_name (abfd, WASM_NAME_SECTION); + if (s != NULL && wasm_scan_name_function_section (abfd, s)) abfd->flags |= HAS_SYMS; return abfd->xvec;