uint8_t array return is causing errors using all scenarios (**, *, &, nothing)

Question

int8_t** FileToColorMap(char* colorfile, int* colorcount)
{
    FILE* fp = fopen (colorfile, "r");
    if (fp==NULL) 
    { 
        printf("no such file."); 
        return 0; 
    } 

    int r,g,b;
    fscanf(fp, "%d", colorcount);
    uint8_t* output;
    output = (uint8_t *)malloc(*colorcount * sizeof(uint8_t));
    for (int i = 0; i < *colorcount;i++) {
        if( fscanf(fp, "%d %d %d", &r, &g, &b) != EOF ) {
            // int* arr= (int *)malloc(3 * sizeof(int));
            // arr[0] = r;
            // arr[1] = g;
            // arr[2] = b;
            int arr[3] = {r,g,b};
            output[i] = *arr;
        } else
        {
            freeMap(i+1, &output);
            return NULL;
        }
        
    }
    fclose(fp);
    return *output;
}

This causes seg fault error, or error 1 or something even when I try return output, **output, &ouput.

It looks like your post is mostly code; please add some more details.

*output --> incompatible pointer types returning 'uint8_t *' (aka 'unsigned char *') from a function with result type 'uint8_t **' (aka 'unsigned char **'); take the address with & [-Wincompatible-pointer-types] — Maryam Shahid, Sep 09 '20 at 06:26
No seg error with &output but abort trap 6 and --> warning: address of stack memory associated with local variable 'output' returned [-Wreturn-stack-address] — Maryam Shahid, Sep 09 '20 at 06:27
Please [edit] your question to add more details. All information should be in the question, not scattered in comments. comments are for suggestions or to ask for clarification. Please write what you want to achieve with this function and show the code that calls this function including the definition of parameters and variable for the return value. — Bodo, Sep 09 '20 at 06:30
I guess the intention is to return an array of `*colorcount` RGB values, i.e. an array of structures or a two-dimensional array. Please clarify this in the question — Bodo, Sep 09 '20 at 06:47
@MaryamShahid To me it's unclear what you are trying to do. There are obvious bugs in the post code but it's hard to fix them when we don't know exactly what you want to do. Please a a description of what you expect the function to do. — Support Ukraine, Sep 09 '20 at 06:49

0___________ · Answer 1 · 2020-09-09T10:27:11.060

There are many issues:

Wrong type and allocation

uint8_t* output;
output = (uint8_t *)malloc(*colorcount * sizeof(uint8_t));

You need to declare pointer to pointer here and allocate space for the pointers not uint8_t

Wrong types of both arrays. arr is automatic variable and dereferencing it outside the function scope is an UB

many other issues

int8_t** FileToColorMap(char* colorfile, int* colorcount)
{
    FILE* fp = fopen (colorfile, "r");
    if (fp==NULL) 
    { 
        printf("no such file."); 
        return 0; 
    } 

    int8_t r,g,b;
    if(fscanf(fp, "%d", colorcount) != 1)
    {
       /* fscanf failed - do something */
    }
    int8_t **output = malloc(*colorcount * sizeof(*output));
    for (int i = 0; i < *colorcount;i++) {
        if( fscanf(fp, "%hhx %hhx %hhx", &r, &g, &b) == 3) {

            uint8_t *arr = malloc(3 * sizeof(*arr));
            arr[0] = r;
            arr[1] = g;
            arr[2] = b;
            output[i] = arr;
        } else
        {
            freeMap(i+1, &output);
            return NULL;
        }
        
    }
    fclose(fp);
    return output;
}

always check the result if malloc. I do not for the clarity sake

Nitpick: The return value from `fscanf(fp, "%d", colorcount);` should be checked but besides that I think this is probably what OP is looking for. — Support Ukraine, Sep 09 '20 at 07:08

Lundin · Answer 2 · 2020-09-09T08:09:46.397

Overall the code is needlessly complicated. I would propose to use a struct instead, since the inner-most dimension in this case is always 3 items wide.

Assuming color_count is allocated by the caller and returns the number of items read, then:

#include <stdio.h>
#include <stdint.h>
#include <stdlib.h>

typedef struct
{
  uint8_t r;
  uint8_t g;
  uint8_t b;
} rgb_t;

rgb_t* FileToColorMap (const char* color_file, int* color_count)
{
  FILE* fp = fopen (color_file, "r");
  if (fp==NULL) 
  { 
    *color_count = 0;
    return NULL;
  } 
  
  int count;
  if(fscanf(fp, "%d", &count) != 1)
  {
    fclose(fp);
    *color_count = 0;
    return NULL;
  }
  
  rgb_t* result = malloc ( sizeof(rgb_t[count]) );
  if(result == NULL)
  {
    fclose(fp);
    *color_count = 0;
    return NULL;
  }
  
  for (int i = 0; i<count; i++) 
  {
    int r,g,b;
    if( fscanf(fp, "%d %d %d", &r, &g, &b) != 3 )
    {
      free(result);
      fclose(fp);
      *color_count = 0;
      return NULL;
    }
    
    result[i].r = r;
    result[i].b = b;
    result[i].g = g;
  }

  fclose(fp);
  *color_count = count;
  return result;
}

Note:

const correctness added to the file name parameter.
Proper clean-up both of dynamic memory and the file handle upon errors.
Use a temporary internal variable count until you know that everything went well. Write to the caller's variable in the end of the function.
Check that the result of fscanf corresponds to the expected number of items, not just that it isn't EOF.

Advanced:

To clean up the code further, one needs to look at the error handling and resource clean-up. There's lots of repetition in the error handling in the above code, which isn't ideal - clean-up should be centralized at one location or it is easy to get resource leaks, particularly during maintenance later on.

There's three ways to do that: a clean-up macro, an "on error goto" pattern or a wrapper function. I prefer the latter since it is the least messy. It's done by breaking out the ownership of resources to an outer function, the one that the caller knows about. Then you put the actual algorithm inside an internal static function.

Here's an example of that, with centralized clean-up/error handling:

#include <stdio.h>
#include <stdint.h>
#include <stdlib.h>
#include <stdbool.h>

typedef struct
{
  uint8_t r;
  uint8_t g;
  uint8_t b;
} rgb_t;

static bool FileReadColorMap (FILE* fp, rgb_t** result, int* color_count)
{
  int count;
  fscanf(fp, "%d", &count);
  
  *result = malloc ( sizeof(rgb_t[count]) );
  if(*result == NULL)
  {
    return false;
  }
  
  for (int i = 0; i < count; i++) 
  {
    int r,g,b;
    if( fscanf(fp, "%d %d %d", &r, &g, &b) != 3 )
    {
      return false;
    }
    
    (*result)[i].r = r;
    (*result)[i].b = b;
    (*result)[i].g = g;
  }

  *color_count = count;
  return true;
}

rgb_t* FileToColorMap (const char* color_file, int* color_count)
{
  rgb_t* result = NULL;
  FILE*  fp     = NULL;
  
  fp = fopen (color_file, "r");
  if(fp == NULL)
  {
    *color_count = 0;
    return NULL;
  }
  
  if(!FileReadColorMap(fp, &result, color_count))
  {
    *color_count = 0;
    free(result);  // it is safe to do free(NULL)
    result = NULL; // return NULL
  }
  
  fclose(fp); // always called
  return result;
}

uint8_t array return is causing errors using all scenarios (**, *, &, nothing)

2 Answers2