[PATCH] scr: fix DEREF_OF_NULL.RET.STAT in ar.c

[Mark Wielaard]

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
-------------- next part --------------
>From 04839d5826d21e7a603a76fddc7afed6d32ab087 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mark@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.moryakov@gmail.com>
Signed-off-by: Mark Wielaard <mark@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