invalid pointer free ()

Question

invalid pointer free ()

Navigation

#1 by (1 votes)
#2 by (0 votes)

3

ERROR: (gdb)
#0  0x00007ffff7a66067 in __GI_raise (sig=sig@entry=6)
    at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x00007ffff7a67448 in __GI_abort () at abort.c:89
#2  0x00007ffff7aa41b4 in __libc_message (do_abort=do_abort@entry=1,
    fmt=fmt@entry=0x7ffff7b99530 "*** Error in '%s': %s: 0x%s ***\n")
    at ../sysdeps/posix/libc_fatal.c:175
#3  0x00007ffff7aa998e in malloc_printerr (action=1,
    str=0x7ffff7b95646 "free(): invalid pointer", ptr=<optimized out>)
    at malloc.c:4996
#4  0x00007ffff7aaa696 in _int_free (av=<optimized out>, p=<optimized out>,
    have_lock=0) at malloc.c:3840
#5  0x0000000000400bb9 in free_all_filenames_array (all_files=0x605010, len=1)
    at directorio.c:24
#6  0x00000000004028a0 in add_all_directory ()
#7  0x0000000000401967 in main (argc=1, argv=0x7fffffffe288) at programa.c:30

this is my function:

void free_all_filenames_array(char **all_files, int len){

  int i;


  for (i = 0; i<len; i++){

      if(all_files[i] != NULL){
       free(all_files[i]); // linea 24
       all_files[i] = NULL;
      }
    }

  if(all_files != NULL){
  free(all_files);
  all_files = NULL;
  }
}

int read_all_filenames_array(char *path, char ***all_files, int *error){

  void ** ptr;
  char **nombres = NULL;
  struct dirent *contenido;
  char realPath[PATH_MAX];
  static int num_nombres = 0;


  DIR *dir  = opendir(path);

  if(dir == NULL){
    printf(" ERROR");
    free(path);
    *error = errno;
    closedir(dir);
    return -1;
  }
//   
  realpath(path,realPath);

  while ((contenido = readdir(dir)) != NULL){

  if( contenido->d_type == DT_REG){// si es regular

    ptr = realloc(nombres, (num_nombres + 1) * sizeof(char *));

    if (ptr == NULL){
       printf("ERROR couldnt reallocate the memory");
      *error = errno;
      free_all_filenames_array(nombres,num_nombres);
      closedir(dir);
      return -1;
    }

    nombres = (char **)ptr;

    strcat(realPath,contenido->d_name);
    nombres[num_nombres] = strdup(realPath);

    if(nombres[num_nombres] == NULL){
       free_all_filenames_array(nombres,num_nombres + 1);
       *error = errno;
       closedir(dir);
       return -1;
    }

    num_nombres++;
  }
 }


if (nombres != NULL){
      *all_files = nombres;
     free_all_filenames_array(nombres,num_nombres); // AQUI ME SUELE DAR EL ERROR
     nombres = NULL;
  }
  else printf("Sin ficheros regulares");

  *error = errno;
  closedir(dir);
  return num_nombres;

 }

c

asked by ALVARO ROJO ORTEGO 26.10.2016 в 19:41

source

2 answers

0

The code that you present has enough conflicting points. I'll list the ones I've been able to locate:

void free_all_filenames_array(char **all_files, int len){

  int i;

  for (i = 0; i<len; i++){

      if(all_files[i] != NULL){ // <<--- 1
       free(all_files[i]); // linea 24
       all_files[i] = NULL; // <<--- 2
      }
    }

  if(all_files != NULL){ // <<--- 3
  free(all_files);
  all_files = NULL; // <<--- 4
  }
}

If free is passed a pointer to NULL does absolutely nothing, then that check you can save it without fear because it does not give you anything of value.

It does not make much sense that you put the pointer to NULL when 4 lines of code below you are going to load that memory area. This is not conflicting but it is a line of code that does not contribute anything.

That check or delete it or move it to the beginning of the function, since either you assume that the function will always be called by passing a correct pointer or else you protect the function from the beginning. As discussed in the first point, this check as is is of no use.

all_files is a local variable of the function ... what you do in that variable is not going to be reflected off later or you convert all_files into a triple pointer or you save this instruction.

More little things:

int read_all_filenames_array(char *path, char ***all_files, int *error){

  char **nombres = NULL;
  static int num_nombres = 0;

It does not make much sense that the element counter in the array stays inside the function. The logical thing is that the arrangement and the counter of elements travel together and the ideal for this situation is to make use of a structure:

typedef struct
{
  char ** nombres;
  int num_nombres;
} ListaNombres;

Why is this a hot spot? Because to be able to do something with the list of names outside the function you need to know how many elements the list has, so having each value on its side complicates the final code.

if(dir == NULL){
  printf(" ERROR");
  free(path); // <<--- 1
  *error = errno; // <<--- 2
  closedir(dir); // <<--- 3
  return -1;
}

This function has not made the reservation of path , the name of the function does not indicate that it should free the memory and path is a simple pointer, then after the function call the Original pointer is dereferenced and can be dangerous.

errno is available from anywhere in the code. If an error occurs it is much more readable to choose to return an easily recognizable error code (the value of an enumerated one for example) or simply indicate that an error has occurred and let the one using the function call errno by your account.

If the file could not be opened, its descriptor is 0. If you try to close a descriptor with value 0, the function does absolutely nothing ... another instruction that is left over.

More details:

ptr = realloc(nombres, (num_nombres + 1) * sizeof(char *));

Bearing in mind that num_nombres is static, every time you call this function you will try to make an increasingly larger reservation, which does not make any sense because the reservation you made in the previous call has been lost. From the second call to the function the result returned will be incorrect.

if (nombres != NULL){
  *all_files = nombres;
 free_all_filenames_array(nombres,num_nombres); // AQUI ME SUELE DAR EL ERROR
 nombres = NULL;
}

Let's analyze this code with peace of mind:

If nombres is not NULL copies the pointer of nombres to all_files : I hope that in the documentation you warn that the possible memory initially targeted by all_files will be dereferenced since it will not be possible to recover it.
Then you release the reserved memory by nombres , so that the pointer all_files is automatically pointed to an invalid memory zone.
You put nombres pointing to NULL , which has no effect because nombres is a local variable.

That code should practically rewrite the entire code because it has several dangerous and dangerous points. Mainly you should focus on what the responsibilities of the function are and stick to them. Constructing monolithic functions is usually not a good option because the resulting code is often complicated to manage.

Ah yes, I forgot ... try to tabulate the code well to be able to differentiate the blocks without having to copy it in a code editor and reindentarlo or have to walk counting the keys.

Greetings.

answered by 27.10.2016 в 22:51

Android Explorer File Explorer does not work Undefined index btn php

score 1 · Accepted Answer

I've been looking for a while, but I think I've found it:

num_nombres++;
} }

if (nombres != NULL){ *all_files = nombres; free_all_filenames_array(nombres,num_nombres); // AQUI ME SUELE DAR EL ERROR nombres = NULL; }else printf("Sin ficheros regulares");

First you increase 'num_names', and then you call 'free_all_filenames_array' with that 'num_names' incremented. I do not have a compiler at hand, but try something like

free_all_filenames_array( nombre, num_nombres - 1 );

If I am right, you are trying to free up memory that you have not assigned.