pointer not allocated

Discussion in 'Mac Programming' started by dougphd, Jul 19, 2016.

  1. dougphd, Jul 19, 2016
    Last edited: Jul 19, 2016

    dougphd macrumors member

    Joined:
    Apr 28, 2016
    #1
    I get a can't be freed because it wasn't allocated at free(day)


    Code:
    typedefstruct sauce
    
    {
    
        char date[14];
    
        char name[20];
    
        float sauce;
    
        float open;
    
        int signal;
    
        int index;
    
        float value;
    
    } pesto;
    
    
    void convert_dates(pesto * signal, int days)
    {
        char  *day, *month, *year;
        char *new_day, *new_month, *new_date;
        int i, j;
        // printf("here again\n");
        new_day = (char *)calloc(3, sizeof (char));
        new_month = (char *)calloc(3, sizeof (char));
        new_date = (char *)calloc(9, sizeof (char));
        day = (char *)calloc(3, sizeof (char));
        month = (char *)calloc(3, sizeof (char));
        year = (char *)calloc(11, sizeof (char));
      
        for(j = 0, i = days - 1062; i < days ; i++, j++)
        {
            // printf("here i %d %s %s\n", i,signal[i].date, month);
            month = strtok(signal[i].date,"/");
          
          
            if (atoi(month) < 10)
            {
                strcpy(new_month,"0");
                strcat(new_month,month);
            }
            else
                strcpy(new_month,month);
          day = strtok(NULL,"/");
            //       printf("day %s\n", day);
           if (atoi(day) < 10)
            {
                strcpy(new_day,"0");
                strcat(new_day,day);
            }
            else
                strcpy(new_day,day);
            //  printf("new_day %d %s\n",i, new_day);
             year = strtok(NULL,"/");
          if ( (atoi(year) > 0 ) && (atoi(year)< 90) )
                strcpy(new_date,"20");
            else
                strcpy(new_date,"19");
            strcat(new_date, year);
            strcat(new_date, new_month);
            strcat(new_date, new_day);
            strcpy(signal[i - 3300].date,new_date);
            signal[j].signal = signal[i].signal;
            //  printf("new_date %s  %d %d \n", new_date,j,   signal[j].signal);
        }
        free(new_day);
        free(new_month);
        free(new_date);
        free(day);
        free(month);
        free(year);
        return;
    }
    
     
  2. dylanryan macrumors member

    Joined:
    Aug 21, 2011
    #2
    On this line:

    day = strtok(NULL,"/");

    you discard the previous value of day (leaking the allocated memory since that was your only pointer to it), and replace it with new data from strtok (which is not your responsibility to clean up). By the time you get to free(day), you are trying to free the return value of some strtok call, and have already long since leaked the memory you calloc'd.

    Ditto for month and year, just that you tried to free day first so it is the one that broke. You probably don't need to calloc any of those, just declare them as pointers and then don't free them.
     
  3. dougphd thread starter macrumors member

    Joined:
    Apr 28, 2016
    #3
    Didn't help. Got different error. Also tried replacing day = strtok(NULL,"/"); with str(day,strtok(NULL,"/"));. That didn't help. Tried to delete message while I try to digest all this.
    Thanks.
     
  4. dylanryan, Jul 19, 2016
    Last edited: Jul 19, 2016

    dylanryan macrumors member

    Joined:
    Aug 21, 2011
    #4
    Post your new code? And the different error? I can't debug what I can't see.

    I would try to run it myself, but as written there are numerous gotchas in the code. How many elements are in signal? What value is days? It looks like days must be 4362 or larger (and signals must have at least days elements), and I do not care to generate 4362 dummy pesto objects to test with.
     
  5. dougphd thread starter macrumors member

    Joined:
    Apr 28, 2016
    #5
    Put code back. When I tried your suggestion, I got an EXC_BAD_ACCESS error here
    if ( (atoi(year) > 0 ) && (atoi(year)< 90) )

    strcpy(new_date,"20");
    --- Post Merged, Jul 19, 2016 ---
    --- Post Merged, Jul 19, 2016 ---
    I would like to learn about the gotchas if possible. If would want to test the code, you can always reduce the number of days. It would be appreciated.
    --- Post Merged, Jul 19, 2016 ---
    I think the error is buried somewhere else in the code.
     
  6. dylanryan macrumors member

    Joined:
    Aug 21, 2011
    #6
    If you got an EXEC_BAD_ACCESS on that atoi, i suspect we did fix the one problem and hit a new one, because the only way you could get that is if STRTOK returned null, meaning you probably tried to tokenize something that doesn't make sense to it based on your other strtok calls (in particular, something with less than 2 '/' characters in it)


    The main problem is that it is hard to edit the function to run on data I make because I do not know exactly what it is supposed to do, and a lot of it looks "like magic" to me -- indices clearly have significance, but without knowing what, I have no idea what the goal is.

    What might help: I will describe in words what it looks like the function is doing. If the words don't match what you want, then we found a bug to squash. Sometimes it helps to see what someone else thinks your code does, since it is easy to go code-blind and not notice things in the code.

    So, here we go:

    convert_dates must be passed in an array of at least 4362 pesto objects, and days is length of the signal array. Any less than 4362 objects and it will crash at best, and corrupt other data on the stack at worst. Sizes larger than 4362 look buggy to me, but it might be intended.

    The function is going to do extensive modification to the passed in object, but what it does is strongly dependent on the length of the array.

    The meat of the loop (which is run exactly 1062 times no matter the length of the signal array) converts dates.
    The input format M/D/Y, where M and D might or might not be 0-padded, and Y might be any number of digits (but probably 2 or 4).
    The output format is YYYYMMDD, where M and D are 0-padded to be 2 digits each, and Y usually 4 digits but not always. Input dates between 1 and 89 inclusive are converted to 20YY, and any dates larger than 90 have the two digits "19" prepended. So, if the initial year happened to be "2000", the output year will be "192000". This leads me to guess that the input data always uses 2-digit years, but without seeing it, I have to guess. (Negative input years, like say -1, would be converted to 19-1. I assume that this does not happen in the data the program is meant to run on, but since you do check for year>0, I am less sure on that assumption. But I doubt you want 19-1). A year of 0 (not 00) converts to 190, so you can have a 3-digit year.

    So, that is the conversion. Now, where that converted date is supposed to go is what strongly depends on input size.

    If the input length is 4362, then element 0 of the array will have its date set to to the converted date from element 3300 in the array. Also, element 0 of the array will have its signal set to the signal of element 3300. The loop then progresses forward, setting the date and signal of element 1 to that of 3301, and so on. This seems more or less ok (though moving some data back 3300 elements seems a little odd, but not immediately standing out as buggy)

    If the input length is 4363, as above, but this time we set the date of element 1 and the signal of element 0 from the element at 3301 (so, we split them up rather than assigning them to the same object). Then the date of 2 and the signal of 1 are copied from 3302. And so on.

    If the input length is 4634, as above, but the date from 3302 goes to 2, and the signal from 3302 goes to 0. Then the date on 3 and the signal on 0 are copied from 3303, and so on.

    And so on

    So, for every element in the input array over 4362, the date and signal are copied that many elements apart at the start of the array (without knowing what exactly you WANT, this feels very buggy to me)


    If the input length is less than 4362, bad things happen. Lets look at length 4361. In that case, i will start at 4361-1062=3299. And then at the bottom of the loop, when this line is called

    strcpy(signal[i - 3300].date,new_date);

    we'll try to set signal[-1]'s date. As I said above: best case scenario, that crashes. Worst case, it doesn't crash, silently corrupts your memory, and almost anything could happen. (this also feels buggy to me, but again, I don't know your goal, so maybe it isn't)

    Because of this length dependence, i can't change the length without also changing the behavior, and without knowing what behavior is intended, doing that might cause the bug you see not to manifest.



    Do those words more or less match the intent of the function? I specifically called out some special cases that might not apply. The date conversion, for example, is perfectly fine if the input data ALWAYS uses exactly 2-digit non-negative years. It only misbehaves on 1- or 3+-digit years, or negative years. But I don't know your data, so I cannot verify that, so I called out the places it fails. And maybe the input is always 4362 elements long, in which case the things that feel buggy aren't functionally a problem (but are still a problem because if the function is used on future data that is different it will explode)

    If not, where are they different? I might have misread the code, or the code might be doing things wrong.
     
  7. robvas macrumors 68020

    Joined:
    Mar 29, 2009
    Location:
    USA
    #7
    Why the heck aren't you just using char arrays? There are reasons to use dynamically allocated memory, and this ain't one of them.

    --- Post Merged, Jul 20, 2016 ---
    What do you think is going on here?

    day = (char *)calloc(3, sizeof (char));

    for(j = 0, i = days - 1062; i < days ; i++, j++)


    What do you think the value of days-1062 is going to be?
     
  8. dylanryan macrumors member

    Joined:
    Aug 21, 2011
    #8
    It'll be 1062 less than the passed in days argument. *days* is completely different than *day*. That said, I agree with the general suggestion of avoiding the heap where possible.
     
  9. robvas macrumors 68020

    Joined:
    Mar 29, 2009
    Location:
    USA
    #9
    Ahh, you're right. Hooray for confusing variable names!
     

Share This Page