[Bug debuginfod/28204] extend webapi / verification with forthcoming signed-contents archives

2021-09-08 Thread fche at redhat dot com via Elfutils-devel 
https://sourceware.org/bugzilla/show_bug.cgi?id=28204

--- Comment #3 from Frank Ch. Eigler  ---
> How would this be used together with debuginfod?
> Where/how would the user get the signatures

>From debuginfod, possibly via additional response headers,
when extracting files from IMA-signed archives.

> and how would the client library check those signatures?

If it has the file and the signature (and the appropriate
public key!) available, the debuginfod client can perform
this integrity check at download time.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

Re: [Bug debuginfod/28034] client-side %-escape url characters

2021-09-08 Thread Mark Wielaard 
Hi Noah,

This looks good, but the description is not really what the patch does.
It is part of the PR28034 fix, but repeating that problem statement
doesn't really make clear what this patch is about. Could explain in
the commit message that curl_easy_escape () also escapes '/' to '%2F'
and that we want to reverse that back to '/'?

Also some (tiny) optimization hints.

This can be done outside/before the loop

  /* Initialize each handle.  */
  for (int i = 0; i < num_urls; i++)

So you only need to escape once. You of course then need to make sure
the escaped_string is freed after the loop.

We already check that the first char is a '/'. It seems silly to curl
escape that one and then unescape it again. So maybe curl_easy_escape
(data[i].handle, filename + 1, 0) and then change the snprintf pattern
to "%s/%s/%s/%s"?
^ the slash got readded here.

The strlen inside the while loop can also be done outside and then
calculated instead of running strlen on the tail every time.

size_t escaped_strlen = strlen (escaped_string);
while( (loc = strstr(loc, "%2F")) )
  {
 loc[0] = '/';
 // pull the string back after replacement
 escaped_strlen -= (loc + 3) - escaped_string);
 memmove(loc+1, loc+3, escaped_strlen + 1);
  }

(Note, not even compiled, so I might be wrong)

Lastly I assume there are already testcases that cover this
functionality? Just wanting to know how you tested it.

Thanks,

Mark

Re: [Bug debuginfod/27277] Describe retrieved files when verbose

2021-09-08 Thread Mark Wielaard 
Hi Noah,

On Wed, 2021-08-25 at 14:08 -0400, Noah Sanci via Elfutils-devel wrote:
> There appear to exist use cases that intend to simply check for the
> existence of content in a debuginfod server, without actually
> downloading it.  In HTTP land, the HEAD operation is the natural
> expression of this.
> Instead of implementing a HEAD/describe option, allow users, with
> enough
> verbosity, to print the HTTP response headers upon retrieving a file.
> E.g output:
> 
> HTTP/1.1 200 OK
> Connection: Keep-Alive
> Content-Length: 2428240
> Cache-Control: public
> Last-Modified: Sat, 15 May 2021 20:49:51 GMT
> Content-Type: application/octet-stream
> Date: Tue, 03 Aug 2021 18:50:36 GMT
> 
> Note: the new test is mostly compatible with the nsanci/test-fix
> commit. When nsanci/test-fix merges with master I can resubmit the
> patch with the test properly rebased and adjusted for maximum
> efficiency.

Please do resubmit the test now that we have separate debuginfod tests.

> The HEAD functionality will (likely) be put into a new PR for
> existing checking.

Thanks. That is https://sourceware.org/bugzilla/show_bug.cgi?id=28284

> From 6351258d707337b69563d4be8effbb30fc42f784 Mon Sep 17 00:00:00
> 2001
> From: Noah Sanci 
> Date: Wed, 28 Jul 2021 14:46:05 -0400
> Subject: [PATCH] debuginfod: PR27277 - Describe retrieved files when
> verbose
> 
> There appear to exist use cases that intend to simply check for the
> existence of content in a debuginfod server, without actually
> downloading it.  In HTTP land, the HEAD operation is the natural
> expression of this.
> Instead of implementing a HEAD/describe option,

Maybe drop this first part, it describes what is not implemented in
this patch.

>  allow users, with enough
> verbosity, to print the HTTP response headers upon retrieving a file.
> E.g output:
> 
> HTTP/1.1 200 OK
> Connection: Keep-Alive
> Content-Length: 2428240
> Cache-Control: public
> Last-Modified: Sat, 15 May 2021 20:49:51 GMT
> Content-Type: application/octet-stream
> Date: Tue, 03 Aug 2021 18:50:36 GMT
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=27277
> 
> Signed-off-by: Noah Sanci 
> [...]
> diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
> index 530f7dc7..cb9e50c7 100644
> --- a/debuginfod/ChangeLog
> +++ b/debuginfod/ChangeLog
> @@ -4,6 +4,17 @@
>   * debuginfod.cxx (handler_cb): Fix after_you unique_set key
>   to the entire incoming URL.
>  
> +2021-08-02  Noah Sanci  
> +
> + PR27277
> + * debuginfod-client.c (struct debuginfod_client): New field 
> + winning_headers.

It seems technically we don't need winning_headers at this point. But
it will be useful when you implement the other PR.

> + (struct handle_data): New field response_data.
> + (header_callback): Store received headers in response_data.
> + (debuginfod_query_server): Activate CURLOPT_HEADERFUNCTION.
> + Save winning response_data.
> + (debuginfod_end): free client winning headers.
> +
> [...]
> +static size_t
> +header_callback (char * buffer, size_t size, size_t numitems, void * 
> userdata)
> +{
> +  if (size != 1)
> +return 0;
> +  /* Temporary buffer for realloc */
> +  char *temp = NULL;
> +  size_t userlen = 0;
> +  if (*(char**)userdata == NULL)
> +{
> +  temp = malloc(numitems+1);
> +  if (temp == NULL)
> +return 0;
> +  memset(temp, '\0', numitems+1);
> +}
> +  else
> +{
> +  userlen = strlen(*(char**)userdata);
> +  temp = realloc(*(char**)userdata, userlen + numitems + 1);
> +  if (temp == NULL)
> +   return 0;
> +}
> +  strncat(temp, buffer, numitems);
> +  *(char**)userdata = temp;
> +  return numitems;
> +}

I am slightly confused about this function.
Is the idea that this is given (part of) the headers and allocates
memory for userdata to store the parts it wants/needs?

If so, do we need to filter the headers here for those that interest
us, or is that simply all headers?

Do we need to protect against using too much memory?
Currently we keep reallocing till we run out of memory, maybe set some
predetermined limit? (64K of headers should be enough for anybody...)

Why do you clear the temp memory the first time, but don't clear the
end of the realloc data? Is the clearing of the whole block necessary
given that the temp data is overwritten immediately with the buffer
contents?

>  /* Query each of the server URLs found in $DEBUGINFOD_URLS for the file
> with the specified build-id, type (debuginfo, executable or source)
> and filename. filename may be NULL. If found, return a file
> @@ -936,10 +966,13 @@ debuginfod_query_server (debuginfod_client *c,
> curl_easy_setopt (data[i].handle, CURLOPT_LOW_SPEED_LIMIT,
>   100 * 1024L);
>   }
> +  data[i].response_data = NULL;
>curl_easy_setopt(data[i].handle, CURLOPT_FILETIME, (long) 1);
>curl_easy_setopt(data[i].handle, CURLOPT_FOLLOWLOCATION, (long) 1);
>curl_easy_setopt(data[i].hand

[Bug libdw/28220] dwarf_location_attr returns high-bit junk from .debug_addr when fetching 32-bit addresses

2021-09-08 Thread mark at klomp dot org via Elfutils-devel 
https://sourceware.org/bugzilla/show_bug.cgi?id=28220

--- Comment #4 from Mark Wielaard  ---
Turns out we could have caught this with the varlocs testcase.
Before the fix:

$ for i in testfile-vars-*.o; do echo $i; tests/varlocs --debug --exprlocs -e
$i | grep exprloc; done
testfile-vars-clang-dwarf4-32.o
location (exprloc) {addr(0x0)}
location (exprloc) {addr(0x4)}
testfile-vars-clang-dwarf4-64.o
location (exprloc) {addr(0x0)}
location (exprloc) {addr(0x4)}
testfile-vars-clang-dwarf5-32.o
location (exprloc) {addr: 0x4}
location (exprloc) {addr: 0x616c6304}
testfile-vars-clang-dwarf5-64.o
location (exprloc) {addr: 0x0}
location (exprloc) {addr: 0x4}
testfile-vars-gcc-dwarf4-32.o
location (exprloc) {addr(0x0)}
location (exprloc) {addr(0x4)}
testfile-vars-gcc-dwarf4-64.o
location (exprloc) {addr(0x0)}
location (exprloc) {addr(0x4)}
testfile-vars-gcc-dwarf5-32.o
location (exprloc) {addr(0x0)}
location (exprloc) {addr(0x4)}
testfile-vars-gcc-dwarf5-64.o
location (exprloc) {addr(0x0)}
location (exprloc) {addr(0x4)}

With the patch:

$ for i in testfile-vars-*.o; do echo $i; LD_LIBRARY_PATH=libelf:libdw
tests/varlocs --debug --exprlocs -e $i | grep exprloc; done
testfile-vars-clang-dwarf4-32.o
location (exprloc) {addr(0x0)}
location (exprloc) {addr(0x4)}
testfile-vars-clang-dwarf4-64.o
location (exprloc) {addr(0x0)}
location (exprloc) {addr(0x4)}
testfile-vars-clang-dwarf5-32.o
location (exprloc) {addr: 0x0}
location (exprloc) {addr: 0x4}
testfile-vars-clang-dwarf5-64.o
location (exprloc) {addr: 0x0}
location (exprloc) {addr: 0x4}
testfile-vars-gcc-dwarf4-32.o
location (exprloc) {addr(0x0)}
location (exprloc) {addr(0x4)}
testfile-vars-gcc-dwarf4-64.o
location (exprloc) {addr(0x0)}
location (exprloc) {addr(0x4)}
testfile-vars-gcc-dwarf5-32.o
location (exprloc) {addr(0x0)}
location (exprloc) {addr(0x4)}
testfile-vars-gcc-dwarf5-64.o
location (exprloc) {addr(0x0)}
location (exprloc) {addr(0x4)}

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[PATCH] lib: Fix unused parameter warning in lib/error.c

2021-09-08 Thread Colin Cross via Elfutils-devel 
Mark the errnum parameter with __attribute__((unused)).

Signed-off-by: Colin Cross 
---
 lib/error.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/error.c b/lib/error.c
index 75e964fd..651a0f3d 100644
--- a/lib/error.c
+++ b/lib/error.c
@@ -35,7 +35,7 @@
 
 unsigned int error_message_count = 0;
 
-void error(int status, int errnum, const char *format, ...) {
+void error(int status, int errnum __attribute__ ((unused)), const char 
*format, ...) {
   va_list argp;
 
   va_start(argp, format);
-- 
2.33.0.153.gba50c8fa24-goog

[PATCH] libdw: set address size, offset size and version on fake CUs

2021-09-08 Thread Mark Wielaard 
There are three "fake CUs" that are associated with .debug_loc,
.debug_loclist and .debug_addr.  These fake CUs are used for "fake
attributes" to provide values that are stored in these sections
instead of in the .debug_info section. These fake CUs didn't have the
address size, offset size and DWARF version set. This meant that
values that depended on those properties might not be interpreted
correctly. One example was the value associated with a DW_OP_addrx
(which comes from the .debug_addr section).

Add a testcase using varlocs to test that addresses can correctly be
retrieved for gcc/clang, DWARF4/5 and 32/64 bits objects.

https://sourceware.org/bugzilla/show_bug.cgi?id=28220

Signed-off-by: Mark Wielaard 
---
 libdw/dwarf_begin_elf.c   |  32 ++--
 tests/Makefile.am |  13 ++-
 tests/run-varlocs-vars.sh |  93 ++
 tests/testfile-vars-clang-dwarf4-32.o.bz2 | Bin 0 -> 568 bytes
 tests/testfile-vars-clang-dwarf4-64.o.bz2 | Bin 0 -> 605 bytes
 tests/testfile-vars-clang-dwarf5-32.o.bz2 | Bin 0 -> 741 bytes
 tests/testfile-vars-clang-dwarf5-64.o.bz2 | Bin 0 -> 761 bytes
 tests/testfile-vars-gcc-dwarf4-32.o.bz2   | Bin 0 -> 660 bytes
 tests/testfile-vars-gcc-dwarf4-64.o.bz2   | Bin 0 -> 691 bytes
 tests/testfile-vars-gcc-dwarf5-32.o.bz2   | Bin 0 -> 728 bytes
 tests/testfile-vars-gcc-dwarf5-64.o.bz2   | Bin 0 -> 768 bytes
 11 files changed, 130 insertions(+), 8 deletions(-)
 create mode 100755 tests/run-varlocs-vars.sh
 create mode 100644 tests/testfile-vars-clang-dwarf4-32.o.bz2
 create mode 100644 tests/testfile-vars-clang-dwarf4-64.o.bz2
 create mode 100644 tests/testfile-vars-clang-dwarf5-32.o.bz2
 create mode 100644 tests/testfile-vars-clang-dwarf5-64.o.bz2
 create mode 100644 tests/testfile-vars-gcc-dwarf4-32.o.bz2
 create mode 100644 tests/testfile-vars-gcc-dwarf4-64.o.bz2
 create mode 100644 tests/testfile-vars-gcc-dwarf5-32.o.bz2
 create mode 100644 tests/testfile-vars-gcc-dwarf5-64.o.bz2

diff --git a/libdw/dwarf_begin_elf.c b/libdw/dwarf_begin_elf.c
index 9e944b86..a368feb8 100644
--- a/libdw/dwarf_begin_elf.c
+++ b/libdw/dwarf_begin_elf.c
@@ -224,6 +224,23 @@ valid_p (Dwarf *result)
   result = NULL;
 }
 
+  /* We are setting up some "fake" CUs, which need an address size.
+ Check the ELF class to come up with something reasonable.  */
+  int elf_addr_size = 8;
+  if (result != NULL)
+{
+  GElf_Ehdr ehdr;
+  if (gelf_getehdr (result->elf, &ehdr) == NULL)
+   {
+ Dwarf_Sig8_Hash_free (&result->sig8_hash);
+ __libdw_seterrno (DWARF_E_INVALID_ELF);
+ free (result);
+ result = NULL;
+   }
+  else if (ehdr.e_ident[EI_CLASS] == ELFCLASS32)
+   elf_addr_size = 4;
+}
+
   /* For dwarf_location_attr () we need a "fake" CU to indicate
  where the "fake" attribute data comes from.  This is a block
  inside the .debug_loc or .debug_loclists section.  */
@@ -247,8 +264,9 @@ valid_p (Dwarf *result)
= (result->sectiondata[IDX_debug_loc]->d_buf
   + result->sectiondata[IDX_debug_loc]->d_size);
  result->fake_loc_cu->locs = NULL;
- result->fake_loc_cu->address_size = 0;
- result->fake_loc_cu->version = 0;
+ result->fake_loc_cu->address_size = elf_addr_size;
+ result->fake_loc_cu->offset_size = 4;
+ result->fake_loc_cu->version = 4;
  result->fake_loc_cu->split = NULL;
}
 }
@@ -274,8 +292,9 @@ valid_p (Dwarf *result)
= (result->sectiondata[IDX_debug_loclists]->d_buf
   + result->sectiondata[IDX_debug_loclists]->d_size);
  result->fake_loclists_cu->locs = NULL;
- result->fake_loclists_cu->address_size = 0;
- result->fake_loclists_cu->version = 0;
+ result->fake_loclists_cu->address_size = elf_addr_size;
+ result->fake_loclists_cu->offset_size = 4;
+ result->fake_loclists_cu->version = 5;
  result->fake_loclists_cu->split = NULL;
}
 }
@@ -306,8 +325,9 @@ valid_p (Dwarf *result)
= (result->sectiondata[IDX_debug_addr]->d_buf
   + result->sectiondata[IDX_debug_addr]->d_size);
  result->fake_addr_cu->locs = NULL;
- result->fake_addr_cu->address_size = 0;
- result->fake_addr_cu->version = 0;
+ result->fake_addr_cu->address_size = elf_addr_size;
+ result->fake_addr_cu->offset_size = 4;
+ result->fake_addr_cu->version = 5;
  result->fake_addr_cu->split = NULL;
}
 }
diff --git a/tests/Makefile.am b/tests/Makefile.am
index c586422e..22942733 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -143,7 +143,7 @@ TESTS = run-arextract.sh run-arsymtest.sh run-ar.sh newfile 
test-nlist \
run-dwfl-report-elf-align.sh run-addr2line-test.sh \
run-addr2line-i-test.sh run-addr2line-i-lex-test.sh \
run-addr2line-i-demangle-test.sh run-addr2line-alt-debugpath.sh \
-   run-va

[Bug libdw/28220] dwarf_location_attr returns high-bit junk from .debug_addr when fetching 32-bit addresses

2021-09-08 Thread mark at klomp dot org via Elfutils-devel 
https://sourceware.org/bugzilla/show_bug.cgi?id=28220

--- Comment #5 from Mark Wielaard  ---
Proposed patch and testcase:
https://sourceware.org/pipermail/elfutils-devel/2021q3/004149.html

-- 
You are receiving this mail because:
You are on the CC list for the bug.

Re: [Bug debuginfod/27277] Describe retrieved files when verbose

2021-09-08 Thread Mark Wielaard 
Hi Noah,

Looks like I reviewed a slightly older version earlier.  Could you
merge the reviewes/changes for both version in a new version of this
patch?

On Fri, Aug 27, 2021 at 02:38:27PM -0400, Noah Sanci via Elfutils-devel wrote:
> From ed7638571f188e346dd466c195b9ebda028d1c65 Mon Sep 17 00:00:00 2001
> From: Noah Sanci 
> Date: Wed, 28 Jul 2021 14:46:05 -0400
> Subject: [PATCH] debuginfod: PR27277 - Describe retrieved files when verbose
> 
> There appear to exist use cases that intend to simply check for the
> existence of content in a debuginfod server, without actually
> downloading it.  In HTTP land, the HEAD operation is the natural
> expression of this.
> Instead of implementing a HEAD/describe option, allow users, with enough
> verbosity, to print the HTTP response headers upon retrieving a file.

Same comment as earlier, but now please also state the debuginfod
changes itself (the extra headers added).

> E.g output:
> 
> HTTP/1.1 200 OK
> Connection: Keep-Alive
> Content-Length: 2428240
> Cache-Control: public
> Last-Modified: Sat, 15 May 2021 20:49:51 GMT
> X-FILE: "file name"
> X-FILE-SIZE: "byte length of file"

I think an actual output example is better than including "place
holder text".

> +2021-08-02  Noah Sanci  
> +
> + PR27277
> + * debuginfod-client.c (struct debuginfod_client): New field
> + winning_headers.
> + (struct handle_data): New field response_data.
> + (header_callback): Store received headers in response_data.
> + (debuginfod_query_server): Activate CURLOPT_HEADERFUNCTION.
> + Save winning response_data.
> + (debuginfod_end): free client winning headers.
> + * debuginfod.cxx (handle_buildid_f_match): remove X-FILE path. Add
> + X-FILE and X-FILE-SIZE headers.
> + (handle_buildid_r_match): remove X-FILE path. Add X-FILE, X-FILE-SIZE
> + headers, and X-ARCHIVE headers.

So the changes compared to the other one are just in the
debuginfod.cxx file.

> diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
> index 6e182a84..e9b7e76c 100644
> --- a/debuginfod/debuginfod.cxx
> +++ b/debuginfod/debuginfod.cxx
> @@ -1068,6 +1068,9 @@ handle_buildid_f_match (bool internal_req_t,
>else
>  {
>MHD_add_response_header (r, "Content-Type", 
> "application/octet-stream");
> +  std::string file = b_source0.substr(b_source0.find_last_of("/")+1, 
> b_source0.length());
> +  MHD_add_response_header (r, "X-FILE-SIZE", 
> to_string(s.st_size).c_str() );
> +  MHD_add_response_header (r, "X-FILE", file.c_str() );
>add_mhd_last_modified (r, s.st_mtime);
>if (verbose > 1)
>  obatched(clog) << "serving file " << b_source0 << endl;
> @@ -1537,6 +1540,9 @@ handle_buildid_r_match (bool internal_req_p,
>inc_metric ("http_responses_total","result","archive fdcache");
>  
>MHD_add_response_header (r, "Content-Type", 
> "application/octet-stream");
> +  MHD_add_response_header (r, "X-FILE-SIZE", 
> to_string(fs.st_size).c_str());
> +  MHD_add_response_header (r, "X-ARCHIVE", b_source0.c_str());
> +  MHD_add_response_header (r, "X-FILE", b_source1.c_str());
>add_mhd_last_modified (r, fs.st_mtime);
>if (verbose > 1)
>  obatched(clog) << "serving fdcache archive " << b_source0 << " file 
> " << b_source1 << endl;
> @@ -1678,6 +1684,11 @@ handle_buildid_r_match (bool internal_req_p,
>else
>  {
>MHD_add_response_header (r, "Content-Type", 
> "application/octet-stream");
> +  std::string file = b_source1.substr(b_source1.find_last_of("/")+1, 
> b_source1.length());
> +  MHD_add_response_header (r, "X-FILE-SIZE", 
> to_string(fs.st_size).c_str());
> +  MHD_add_response_header (r, "X-ARCHIVE", b_source0.c_str());
> +  MHD_add_response_header (r, "X-FILE", file.c_str());
> +
>add_mhd_last_modified (r, archive_entry_mtime(e));
>if (verbose > 1)
>  obatched(clog) << "serving archive " << b_source0 << " file " << 
> b_source1 << endl;

This looks good. But I wonder if, since these are headers specific to
debuginfod, they shouldn't be named X-DEBUGINFOD-... Just to make sure
they don't clash with any others a proxy might insert.

> +2021-08-04  Noah Sanci  
> +
> + PR27277
> + * debuginfod-find.1: Increasing verbosity describes the downloaded
> + file.
> + * debuginfod.8: Describe X-FILE, X-FILE-SIZE, and X-ARCHIVE.
> +

> diff --git a/doc/debuginfod.8 b/doc/debuginfod.8
> index 5b0d793c..e9c58fbb 100644
> --- a/doc/debuginfod.8
> +++ b/doc/debuginfod.8
> @@ -256,6 +256,13 @@ Unknown buildid / request combinations result in HTTP 
> error codes.
>  This file service resemblance is intentional, so that an installation
>  can take advantage of standard HTTP management infrastructure.
>  
> +Upon finding a file in an archive or simply in the database, some
> +custom http headers are added to the response. For files in the
> +database X-FILE and X-FILE-

Re: Adding a new section to an ELF file aborts with Assertion `shdr != NULL'

2021-09-08 Thread Mark Wielaard 
Hi Alex,

On Sat, Sep 04, 2021 at 08:38:51AM +, Alexander Egorenkov via 
Elfutils-devel wrote:
> i'm facing an issue with libelf when i try to add a new section to an
> ELF file.
> 
> How to reproduce the issue:
> 1. Create a simple ELF file with libelf containing only 2 sections, NULL and
> a string table
> 2. Close ELF file
> 3. Reopen the new ELF file in RW mode with libelf
> 4. Add new section with elf_newscn()
> 5. Update ELF with elf_update()
> 6. Assertion appears
> 
> The weird thing is that if i add only a new program segment then
> everything goes well. But as soon as i add a new section, it fails.
> 
> Error message:
> 
> test: elf32_updatenull.c:214: __elf64_updatenull_wrlock: Assertion `shdr
> != NULL' failed.
> 
> Any hint what i'm doing wrong ? 

I don't immediately know. Could you post the code you are using and/or
the generated file after step 2? That might make it a bit easier to
see exactly what is going on.

Thanks,

Mark

Re: [PATCH] src: add -Wno-error=stack-usage= to AM_LDFLAGS

2021-09-08 Thread Mark Wielaard 
Hi Dmitry,

On Mon, Sep 06, 2021 at 03:59:05AM +0300, Dmitry V. Levin wrote:
> While -Wstack-usage= is already excluded from AM_CFLAGS for various
> tools in src using *_no_Wstack_usage variables, this obviously does not
> help when LTO is enabled, so add -Wno-error=stack-usage= to AM_LDFLAGS
> for linking tools in src.

I didn't know it worked to pass warning flags to the compiler during
linking for lto. That is really useful in this case. Of course maybe
we really should get rid of the unbounded stack allocations in the
first place. But till we do, we need this to build with lto enabled.

Please apply.

Thanks,

Mark

Re: [PATCH] Remove redundant casts of memory allocating functions returning void *

2021-09-08 Thread Mark Wielaard 
Hi Dmitry,

On Mon, Sep 06, 2021 at 08:00:00AM +, Dmitry V. Levin wrote:
> Return values of functions returning "void *", e.g. calloc, malloc,
> realloc, xcalloc, xmalloc, and xrealloc, do not need explicit casts.

This does make the code slightly more readable. And the cast doesn't
really add much useful info. So please apply.

Thanks,

Mark

Re: [PATCH 1/2] Introduce xasprintf

2021-09-08 Thread Mark Wielaard 
Hi Dmitry,

On Mon, Sep 06, 2021 at 10:00:00AM +, Dmitry V. Levin wrote:
> Similar to other x* functions, xasprintf is like asprintf except that
> it dies in case of an error.

Looks useful and the implementation seems correct.
Please apply.

Thanks,

Mark

Re: [PATCH 2/2] Use xasprintf instead of asprintf followed by error(EXIT_FAILURE)

2021-09-08 Thread Mark Wielaard 
Hi Dmitry,

On Mon, Sep 06, 2021 at 10:00:00AM +, Dmitry V. Levin wrote:
> + * color.c (parse_opt): Replace asprintf followed by error(EXIT_FAILURE)
> + with xasprintf.

> + * objdump.c (show_disasm): Replace asprintf followed by
> + error(EXIT_FAILURE) with xasprintf.
> + * readelf.c (handle_gnu_hash): Likewise.
> + * unstrip.c (handle_output_dir_module, main): Likewise.

All these replacements look correct. Please apply.

Thanks,

Mark

Re: [PATCH] findtextrel: do not use unbound alloca

2021-09-08 Thread Mark Wielaard 
Hi Dmitry,

On Mon, Sep 06, 2021 at 06:00:00PM +, Dmitry V. Levin wrote:
> This fixes the following compilation warning:
> 
> findtextrel.c:184:1: warning: stack usage might be unbounded [-Wstack-usage=]

And it simplifies the code a lot IMHO.
Looks good, please apply.

Thanks,

Mark