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

[Mark Wielaard]

Hi Anton,

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

On Thu, 2025-02-13 at 18:00 +0300, Anton Moryakov wrote:
> Report of the static analyzer:
> 1. DEREF_OF_NULL.RET Pointer, returned from function 'elf_getarhdr' at ar.c:498, may be NULL and is dereferenced at ar.c:500.
> 2. DEREF_OF_NULL.RET Pointer, returned from function 'elf_getarhdr' at ar.c:940, may be NULL and is dereferenced at ar.c:943
> 3. DEREF_OF_NULL.RET Pointer, returned from function 'elf_getarhdr' at ar.c:1147, may be NULL and is dereferenced at ar.c:1150.
> 4. DEREF_OF_NULL.RET.STAT Return value of a function 'elf_rawfile' is dereferenced at ar.c:857 without checking for NULL, but it is usually checked for this function (4/5)
> 
> Corrections explained:
> Added check if (arhdr == NULL) goto next;
> 
> Triggers found by static analyzer Svace.
> 
> Signed-off-by: Anton Moryakov <ant.v.moryakov@gmail.com>
> 
> ---
>  src/ar.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/src/ar.c b/src/ar.c
> index 9ace28b9..4b90115d 100644
> --- 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.

Thanks,

Mark