Re: [PATCH] elfclassify tool
* Mark Wielaard:
> + if (elf == NULL)
> +{
> + /* This likely means it just isn't an ELF file, probably not a
> + real issue, but warn if verbose reporting. */
> + if (verbose > 0)
> + fprintf (stderr, "warning: %s: %s\n", current_path, elf_errmsg (-1));
> + return false;
> +}
Is it possible to distinguish the error from a memory allocation error?
It would be wrong to mis-classify a file just because the system is low
on memory.
Thanks,
Florian
Re: [PATCH] elfclassify tool
* Mark Wielaard:
> +/* Called to process standard input if flag_stdin is not no_stdin. */
> +static void
> +process_stdin (int *status)
> +{
> + char delim;
> + if (flag_stdin == do_stdin0)
> +delim = '\0';
> + else
> +delim = '\n';
> +
> + char *buffer = NULL;
> + size_t buffer_size = 0;
> + while (true)
> +{
> + ssize_t ret = getdelim (&buffer, &buffer_size, delim, stdin);
> + if (ferror (stdin))
> + {
> + current_path = NULL;
> + issue (errno, N_("reading from standard input"));
> + break;
> + }
> + if (feof (stdin))
> +break;
> + if (ret < 0)
> +abort (); /* Cannot happen due to error checks above. */
> + if (delim != '\0' && ret > 0)
> +buffer[ret - 1] = '\0';
I think this can overwrite the last character of the last line if the
file does not end with '\n'.
Thanks,
Florian
Re: [PATCH] elfclassify tool
* Mark Wielaard: > Signed-off-by: Mark Wielaard Does elfutils use DCO? Then yoy have my signoff as well: Signed-off-by: Florian Weimer You should you list yourself as an author somewhere in the commit message. This is so much more than what I wrote. Regarding the test case, I think if the build target is ELF, it makes sense to check that the elfutils binaries themselves are classified as expected, with the current build flags. This will detect changes required due to the evolution of the toolchain. Thanks, Florian
Re: [PATCH] elfclassify tool
On Mon, Jul 29, 2019 at 10:43:56AM +0200, Florian Weimer wrote:
> * Mark Wielaard:
>
> > + if (elf == NULL)
> > +{
> > + /* This likely means it just isn't an ELF file, probably not a
> > +real issue, but warn if verbose reporting. */
> > + if (verbose > 0)
> > + fprintf (stderr, "warning: %s: %s\n", current_path, elf_errmsg (-1));
> > + return false;
> > +}
>
> Is it possible to distinguish the error from a memory allocation error?
> It would be wrong to mis-classify a file just because the system is low
> on memory.
You are right this is not the proper way to report the issue.
Normally, when just using elf_begin, a NULL return should be reported
through elf_issue (which will set the issues flag).
But, because I added -z, we are using either elf_begin or
dwelf_elf_begin. dwelf_elf_begin will return NULL (instead of a an
empty (ELF_K_NONE) Elf descriptor when there is an issue, or the
(decompressed) file wasn't an ELF file.
So we should split the error reporting. If we called elf_begin and get
NULL we should call elf_issue to report the proper issue.
If we called dwefl_elf_begin and we get NULL, I am not sure yet what
the proper way is to detect whether it is a real issue, or "just" not
a (decompressed) ELF file. I am afraid the current handling is the
best we can do.
Maybe we can fix dwelf_elf_begin to return an empty (ELF_K_NONE) Elf
descriptor if there was no issue, but the (decompressed) file wasn't
an ELF file.
Cheers,
Mark
Re: [PATCH] elfclassify tool
On Mon, Jul 29, 2019 at 11:16:31AM +0200, Florian Weimer wrote:
> * Mark Wielaard:
>
> > +/* Called to process standard input if flag_stdin is not no_stdin. */
> > +static void
> > +process_stdin (int *status)
> > +{
> > + char delim;
> > + if (flag_stdin == do_stdin0)
> > +delim = '\0';
> > + else
> > +delim = '\n';
> > +
> > + char *buffer = NULL;
> > + size_t buffer_size = 0;
> > + while (true)
> > +{
> > + ssize_t ret = getdelim (&buffer, &buffer_size, delim, stdin);
> > + if (ferror (stdin))
> > + {
> > + current_path = NULL;
> > + issue (errno, N_("reading from standard input"));
> > + break;
> > + }
> > + if (feof (stdin))
> > +break;
> > + if (ret < 0)
> > +abort (); /* Cannot happen due to error checks above. */
> > + if (delim != '\0' && ret > 0)
> > +buffer[ret - 1] = '\0';
>
> I think this can overwrite the last character of the last line if the
> file does not end with '\n'.
I see. "The buffer is null-terminated and includes the newline
character, if one was found."
So the test should be:
diff --git a/src/elfclassify.c b/src/elfclassify.c
index ebd42c1d5..b17d14d45 100644
--- a/src/elfclassify.c
+++ b/src/elfclassify.c
@@ -862,7 +862,7 @@ process_stdin (int *status)
break;
if (ret < 0)
abort (); /* Cannot happen due to error checks above. */
- if (delim != '\0' && ret > 0)
+ if (delim != '\0' && ret > 0 && buffer[ret - 1] == '\n')
buffer[ret - 1] = '\0';
current_path = buffer;
process_current_path (status);
Thanks,
Mark
Re: [PATCH] elfclassify tool
* Mark Wielaard:
> On Mon, Jul 29, 2019 at 11:16:31AM +0200, Florian Weimer wrote:
>> * Mark Wielaard:
>>
>> > +/* Called to process standard input if flag_stdin is not no_stdin. */
>> > +static void
>> > +process_stdin (int *status)
>> > +{
>> > + char delim;
>> > + if (flag_stdin == do_stdin0)
>> > +delim = '\0';
>> > + else
>> > +delim = '\n';
>> > +
>> > + char *buffer = NULL;
>> > + size_t buffer_size = 0;
>> > + while (true)
>> > +{
>> > + ssize_t ret = getdelim (&buffer, &buffer_size, delim, stdin);
>> > + if (ferror (stdin))
>> > + {
>> > +current_path = NULL;
>> > +issue (errno, N_("reading from standard input"));
>> > +break;
>> > + }
>> > + if (feof (stdin))
>> > +break;
>> > + if (ret < 0)
>> > +abort (); /* Cannot happen due to error checks above.
>> > */
>> > + if (delim != '\0' && ret > 0)
>> > +buffer[ret - 1] = '\0';
>>
>> I think this can overwrite the last character of the last line if the
>> file does not end with '\n'.
>
> I see. "The buffer is null-terminated and includes the newline
> character, if one was found."
>
> So the test should be:
>
> diff --git a/src/elfclassify.c b/src/elfclassify.c
> index ebd42c1d5..b17d14d45 100644
> --- a/src/elfclassify.c
> +++ b/src/elfclassify.c
> @@ -862,7 +862,7 @@ process_stdin (int *status)
> break;
>if (ret < 0)
> abort (); /* Cannot happen due to error checks above. */
> - if (delim != '\0' && ret > 0)
> + if (delim != '\0' && ret > 0 && buffer[ret - 1] == '\n')
> buffer[ret - 1] = '\0';
>current_path = buffer;
>process_current_path (status);
Right. But now I wonder why ret == 0 can ever happen. Maybe on
OpenVMS, which doesn't use in-band signaling for line terminators?
Thanks,
Florian
Re: [PATCH] elfclassify tool
On Mon, Jul 29, 2019 at 11:22:13AM +0200, Florian Weimer wrote: > * Mark Wielaard: > > > Signed-off-by: Mark Wielaard > > Does elfutils use DCO? Then yoy have my signoff as well: > > Signed-off-by: Florian Weimer Thanks. Yes, elfutils uses a Developer Certificate of Origin based on the linux kernel one (but clarified for the elfutils project details - like some files having multiple licenses at the same time). We do require all commits to have a Signed-off-by line from developers that agree with the DCO as explained in the CONTRIBUTING document. > You should you list yourself as an author somewhere in the commit > message. The ChangeLog entries do mention both of us as authors. > Regarding the test case, I think if the build target is ELF, it makes > sense to check that the elfutils binaries themselves are classified as > expected, with the current build flags. This will detect changes > required due to the evolution of the toolchain. That is what the run-elfclassify-self.sh testcase does I believe. Or do you believe it should be extended? Thanks, Mark
Re: [PATCH] elfclassify tool
* Mark Wielaard: >> Regarding the test case, I think if the build target is ELF, it makes >> sense to check that the elfutils binaries themselves are classified as >> expected, with the current build flags. This will detect changes >> required due to the evolution of the toolchain. > > That is what the run-elfclassify-self.sh testcase does I believe. Ah, I had missed it. Great, I think this one is covered. Thanks, Florian
