Hi Anton,
On Thu, Feb 27, 2025 at 06:12:36PM +0100, Mark Wielaard wrote:
> The subject isn't super helpful unless you know the specific
> terminology of the statuc analyzer you are using. It would be better to
> say something like:
>
> ar: check whether elf_getarhdr returns NULL
>
> > --- a/src/ar.c
> > +++ b/src/ar.c
> > @@ -498,6 +498,9 @@ do_oper_extract (int oper, const char *arfname, char
> > **argv, int argc,
> > while ((subelf = elf_begin (fd, cmd, elf)) != NULL)
> > {
> > Elf_Arhdr *arhdr = elf_getarhdr (subelf);
> > +
> > + if (arhdr == NULL)
> > + goto next;
> >
>
> OK, but the identation is completely wrong. There is extra whitespace
> on the first line, just remove the line. The if line should be indented
> 6 spaces, the goto line should be indented one tab.
>
> > if (strcmp (arhdr->ar_name, "/") == 0)
> > {
> > @@ -860,6 +863,9 @@ write_member (struct armem *memb, off_t *startp, off_t
> > *lenp, Elf *elf,
> > {
> > /* In case of a long file name we assume the archive header
> > changed and we write it here. */
> > +
> > + if (arhdr == NULL)
> > + goto next;
> > memcpy (&arhdr, elf_rawfile (elf, NULL) + *startp, sizeof (arhdr));
>
> This doesn't make sense to me, does it even compile?
> arhdr is a struct ar_hdr not a pointer to it.
>
> > snprintf (tmpbuf, sizeof (tmpbuf), "/%-*ld",
> > @@ -943,6 +949,9 @@ do_oper_delete (const char *arfname, char **argv, int
> > argc,
> > while ((subelf = elf_begin (fd, cmd, elf)) != NULL)
> > {
> > Elf_Arhdr *arhdr = elf_getarhdr (subelf);
> > +
> > + if (arhdr == NULL)
> > + goto next;
>
> This does make sense, but indentation needs to be fixed like above.
>
> > /* Ignore the symbol table and the long file name table here. */
> > if (strcmp (arhdr->ar_name, "/") == 0
> > @@ -1152,6 +1161,9 @@ do_oper_insert (int oper, const char *arfname, char
> > **argv, int argc,
> > while ((subelf = elf_begin (fd, cmd, elf)) != NULL)
> > {
> > Elf_Arhdr *arhdr = elf_getarhdr (subelf);
> > +
> > + if (arhdr == NULL)
> > + goto next;
>
> Likewise.
I reimplemented this and committed the attached.
Cheers,
Mark
>From 04839d5826d21e7a603a76fddc7afed6d32ab087 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <m...@klomp.org>
Date: Tue, 29 Apr 2025 22:16:58 +0200
Subject: [PATCH 1/3] ar: Check elf_getahdr doesn't return NULL
When elf_getahdr returns NULL we shouldn't even try to handle the ar
header, but immediately go to the next entry.
* src/ar.c (do_oper_extract): If elf_getahdr goto next.
(do_oper_delete): Likewise.
(do_oper_insert): Likewise.
Suggested-by: Anton Moryakov <ant.v.morya...@gmail.com>
Signed-off-by: Mark Wielaard <m...@klomp.org>
---
src/ar.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/ar.c b/src/ar.c
index 9ace28b918d3..03118c4eadbe 100644
--- a/src/ar.c
+++ b/src/ar.c
@@ -498,6 +498,8 @@ do_oper_extract (int oper, const char *arfname, char
**argv, int argc,
while ((subelf = elf_begin (fd, cmd, elf)) != NULL)
{
Elf_Arhdr *arhdr = elf_getarhdr (subelf);
+ if (arhdr == NULL)
+ goto next;
if (strcmp (arhdr->ar_name, "/") == 0)
{
@@ -943,6 +945,8 @@ do_oper_delete (const char *arfname, char **argv, int argc,
while ((subelf = elf_begin (fd, cmd, elf)) != NULL)
{
Elf_Arhdr *arhdr = elf_getarhdr (subelf);
+ if (arhdr == NULL)
+ goto next;
/* Ignore the symbol table and the long file name table here. */
if (strcmp (arhdr->ar_name, "/") == 0
@@ -1152,6 +1156,8 @@ do_oper_insert (int oper, const char *arfname, char
**argv, int argc,
while ((subelf = elf_begin (fd, cmd, elf)) != NULL)
{
Elf_Arhdr *arhdr = elf_getarhdr (subelf);
+ if (arhdr == NULL)
+ goto next;
/* Ignore the symbol table and the long file name table here. */
if (strcmp (arhdr->ar_name, "/") == 0
--
2.49.0
