-3

I'm facing a problem which is driving me crazy !

I have a function, this one :

void    load_weapons3(t_env *e, char *name, int x, t_weapon *w)
{
    char    *tmp;
    char    *fname;
    t_image i;

    fname = NULL;
    tmp = NULL;
    tmp = ft_get_name_without_extention(name);
    if (!tmp)
        return ;
    fname = ft_strcat(tmp, "_fire.xpm");
    free(tmp);
    if (!fname)
        return ;
    i.image = mlx_xpm_file_to_image(e->mlx_ptr, fname, &(i.x), &(i.y));
    if (!i.image)
    {
        (*w).fire = NULL;
        return ;
    }
    else
        (*w).fire = malloc(sizeof(t_weaponfire) * QTY_OF_FIRE);
    i.image_data = mlx_get_data_addr(i.image,
                                        &(i.bpp),
                                        &(i.size_line),
                                        &(i.endian));
    i.image_tab = get_image_tab(i);
    load_weapon_fire(e, x, i);
    printf("%s\n", fname);
    free(fname);
}

Other parts of code that may be relevant :

int     ft_strlen(char *str)
{
    int     i;

    i = 0;
    while (str[i])
        i++;
    return (i);
}

char    *ft_strcpy(char *str)
{
    int     i;
    int     j;
    char    *cpystr;

    j = 0;
    i = ft_strlen(str);
    cpystr = malloc(sizeof(char) * (i + 1));
    while (j < i)
    {
        cpystr[j] = str[j];
        j++;
    }
    cpystr[j] = '\0';
    return (cpystr);
}
char    *ft_get_name_without_extention(char *fullpath)
{
    char    *str;
    int     i;

    i = ft_strlen(fullpath);
    str = ft_strcpy(fullpath);
    while (i)
    {
        if (str[i] == '.')
        {
            str[i] = '\0';
            return (str);
        }
        i--;
    }
    free(str);
    return (NULL);
}

char    *ft_strcat(char *str1, char *str2)
{
    int     i;
    int     len1;
    int     len2;
    char    *str;

    i = 0;
    str = NULL;
    if (!str1 || !str2)
        return (NULL);
    len1 = ft_strlen(str1);
    len2 = ft_strlen(str2);
    str = malloc(sizeof(char) * (len1 + len2 + 1));
    if (!str)
        return (NULL);
    while (i < len1)
        str[i] = str1[i++];
    len1 = 0;
    while (len1 < len2)
        str[i + len1] = str2[len1++];
    str[i + len1] = '\0';
    return (str);
}

void    load_weapons(t_env *e)
{
    int             xpm_q;
    DIR             *d;
    struct dirent   *dir;

    xpm_q = ft_get_xpm_quantity("img/weapons");
    printf("Xpm_q is : %d\n", xpm_q);
    if (xpm_q > 0)
    {
        e->weapons.weapons_count = xpm_q;
        e->weapons.weapons = malloc(sizeof(t_image) * (xpm_q + 1));
        xpm_q--;
        d = opendir("img/weapons");
        if (d)
        {
            while ((dir = readdir(d)) != NULL)
            {
                load_weapons2(&xpm_q, &(e->weapons.weapons[xpm_q]), e, dir->d_name);
            }
            closedir(d);
        }
    }
    e->weapons.selected_weapon = 0;
}

void    load_weapons2(int *xpm_quantity, t_weapon *w, t_env *e, char *n)
{
    char    *fname;
    t_image *i;

    if (!ft_have_extension(".xpm\0", n) || ft_have_extension("_fire.xpm\0", n))
        return ;
    i = &(w->image);
    fname = ft_strcat("img/weapons/", n);
    i->name = ft_strcpy(n);
    i->image = mlx_xpm_file_to_image(e->mlx_ptr, fname, &(i->x), &(i->y));
    i->image_data = mlx_get_data_addr(i->image,
                                            &(i->bpp),
                                                &(i->size_line),
                                                    &(i->endian));
    i->image_tab = get_image_tab((*i));
    load_weapons3(e, fname, *xpm_quantity, w);
    free(fname);
    (*xpm_quantity)--;
}

And sometimes (really randomly) I get a "double free or corruption (out)", that appears to occur when I free fname pointer. The fact is I'm not double freeing it, and printf prints it without any problem...

If someone has a clue...

I'm using gcc (Ubuntu 4.8.4-2ubuntu1~14.04) 4.8.4, running in VirtualBox.

Thanks for reading !

GlobCoder
  • 21
  • 5
  • 2
    You should post *all* relevant code. For example, *does* `ft_get_name_without_extention()` allocate a buffer big enough to concatenate `"_fire.xpm"` without overflowing? – EOF Aug 24 '15 at 16:12
  • Not sure what you are trying to do, but try removing `if (fname) free(fname);` as `fname` wasn't allocated by `malloc` or family. – Spikatrix Aug 24 '15 at 16:13
  • where did you allocate memory to `fname` ? – ameyCU Aug 24 '15 at 16:14
  • @CoolGuy, ameyCU: `fname` is the same as `tmp` after `ft_strcat()`, provided it works like `strcat()` and `tmp != 0` before, otherwise the test for `if (!fname)` comes too late. – EOF Aug 24 '15 at 16:17
  • Does `ft_get_name_without_extention()` allocate memory? What is returned if it fails? Can `ft_strcat()` take a null pointer, and if so, what does it do? Also, does `ft_strcat()` allocate any memory? – donjuedo Aug 24 '15 at 16:18
  • @EOF If that's the case then why freeing both `tmp` and `fname` ? – ameyCU Aug 24 '15 at 16:18
  • 2
    Going to highly, highly recommend using `valgrind` here. Use `--leak-check=yes` for more detailed info, check out the quick-start guide [here](http://valgrind.org/docs/manual/quick-start.html). – tonysdg Aug 24 '15 at 16:18
  • I updated my question with more code. If I "remove if (fname) free(fname);" everything runs fine... – GlobCoder Aug 24 '15 at 16:19
  • @GlobCoder, thanks for posting code. Does `ft_strcpy()` allocate, too? – donjuedo Aug 24 '15 at 16:20
  • 4
    @GlobCoder: No, everything is **not** fine just because you don't immediately crash after removing that bit of code. The fact that it crashes *with* the code *strongly* suggests the code is as horrifically broken as it looks. Also, sill missing code. – EOF Aug 24 '15 at 16:20
  • @GlobCoder, I have a suggestion. To see if your stack is corrupted, print the hex value of `fname` right after allocation (returned from `ft_strcat()`) and again right before it's freed. – donjuedo Aug 24 '15 at 16:29
  • @donjuedo: No. Nononononono. **NO**. This sort of thing leads to madness. Just use `valgrind` or `address sanitizer`. – EOF Aug 24 '15 at 16:31
  • I reupdated my code, modifying things that wasn't tested... I'll try with valgrind ! Thanks – GlobCoder Aug 24 '15 at 16:32
  • @EOF, I agree that using valgrind is a great idea. But why so much emphasis to not examine the value? Will he go blind? ;-) – donjuedo Aug 24 '15 at 16:58
  • @donjuedo: Because the pointer itself may very well point where it should. The `double free or corruption`-error indicates corruption in `malloc()`s data-structures, which are commonly in between the allocations on the heap. They are usually corrupted by out-of-bounds writes, and the error only surfaces much later, when a memory-allocation function (particularly `free()`) is called. – EOF Aug 24 '15 at 17:02
  • @EOF, Your scenario is certainly one way for things to go wrong, but not the only way. To print values as I suggested and see a match only means to keep looking. It's the case of a mismatch you are excluding, and I simply suggest to look. – donjuedo Aug 24 '15 at 17:06
  • @donjuedo: `fname`s address is never taken (meaning it could have been declared with the `register`-specifier), and it is only assigned to **once**. The compiler would be well within its rights to optimize out *any* check for a change in `fname`, since, barring undefined behavior, it **cannot** change. – EOF Aug 24 '15 at 17:08
  • And yet, I have seen it happen, which is what led to my suggestion. – donjuedo Aug 24 '15 at 17:10
  • It's just getting really strange... After launching my program inside valgrind, it detected some errors and essentially one I corrected. It was about affectation of "e->weapons.weapons[x]", saying "Address 0x6410a78 is 0 bytes after a block of size 56 alloc'd" But I was only accessing [0], so I checked, double checked, and I still can't figure it out, but when I allocate it with one more element (a total of 2 in my case), it works...even if I'm only accessing first element ([0] of course...) I will update with more code... – GlobCoder Aug 24 '15 at 19:15
  • Allocation is done in load_weapons btw – GlobCoder Aug 24 '15 at 19:20
  • @GlobCoder, Did `valgrind` say anything about "double free or corruption"? And are you still getting that after your edits? – donjuedo Aug 24 '15 at 19:39
  • Recommend changing `str[i] = str1[i++];` --> `str[i] = str1[i]; i++;` – chux - Reinstate Monica Aug 24 '15 at 19:54
  • @chux Just to be sure, Doesn't `str[i] = str1[i++];` invoke UB? – Spikatrix Aug 25 '15 at 10:13
  • @Cool Guy `Yes `str[i] = str1[i++]` is UB as it is not defined when the `i` increment occurs relative to the usage of `i` as an index. – chux - Reinstate Monica Aug 25 '15 at 14:35
  • @chux Does str[i + len1] = str2[len1++]; give UB too ? Thx for the advice ! – GlobCoder Aug 25 '15 at 17:38
  • `str[i + len1] = str2[len1++];` has the same problem as `str[i] = str1[i++]` and `foo(i) = bar(i++)`. – chux - Reinstate Monica Aug 25 '15 at 18:01
  • See https://stackoverflow.com/questions/2902064/how-to-track-down-a-double-free-or-corruption-error-in-c-with-gdb – Raedwald Dec 06 '18 at 13:37

1 Answers1

1

Your code is horrible, and you still haven't posted your typedefs and struct-definitions, which will become relevant in the following rant:

So, in load_weapons(), you malloc() an array,

e->weapons.weapons = malloc(sizeof(t_image) * (xpm_q + 1));

the contents of which are presumably supposed to be of type t_image. Then you pass a pointer to the second-to-last valid object of the array to load_weapons2() (great, descriptive name),

load_weapons2(&xpm_q, &(e->weapons.weapons[xpm_q]), e, dir->d_name);

but wait! What was load_weapon2()'s prototype again?

void load_weapons2(int *, t_weapon *, t_env *, char *)

that's no t_image*, that's a t_weapon*! Shock and awe, you then somehow extract a t_image* out of a t_weapon* that was really a t_image* in the first place,

t_image *i;
i = &(w->image);

The only way that last line makes sense is if t_weapon has a member t_image, which obviously necessitates sizeof(t_weapon) >= sizeof(t_image). So, unless t_image is the only member of t_weapon, you've allocated insufficient space.

And now for some completely unsolicited advice: complete rewrite.

EOF
  • 6,273
  • 2
  • 26
  • 50
  • A rewrite would certainly provide benefit. I'd focus on descriptive names, as you pointed out, plus meaningful comments, as well as something as basic as neat formatting to help readability. Learning to code better usually starts with some pretty rough work, and I know I've written my share. I learned, though. – donjuedo Aug 24 '15 at 21:37
  • You are totally right, problem came from allocation of e->weapons.weapons, which was a t_weapon, and obviously not a t_image. I know my code is horrible, it's a schoolwork and we must respect a norm, limitating us to 25 lines for a function and not more than 5 functions per file. So trying to shorten it makes things a little bit complicated sometimes, and even if code works, a look from outside can be very difficult... Nevertheless, I really appreciate for your reading and precise understanding of my code. I'll rewrite all of it for another 2D library soon. A better shrink should be helpful :) – GlobCoder Aug 24 '15 at 22:59
  • Using `malloc(n * sizeof var)` instead of `malloc(n * sizeof type)` avoids the problem as in `e->weapons.weapons = malloc((xpm_q + 1) * sizeof *(e->weapons.weapons));` – chux - Reinstate Monica Aug 25 '15 at 14:39