Freeing struct with unclosed file in C

I don't have much experience in C, my question is as follows:

if I have a struct like this:

typedef struct {
  FILE* file;
  ...
  ...
} MyStruct;

and a function to free this struct:

void free_mystruct(MyStruct* s) {
  if (!s) {
    return;
  }

  if (!s->file) {
    free(s);
    s = NULL;
    return;
  }

  if (fclose(s->file) != 0) {
    // it failed to close the file...
    // how to handle the free?
  }

  free(s);
  s = NULL;
}

does freeing the struct anyways inside the if block where it fails to close the file leak memory or system resources? if it does how to handle the free in a situation like this should I report that it failed and don't perform the free() call and return?

I think this question comes down to the situations where fclose() fails, probably. I don't know these.

EDIT: this is how MyStruct is initialized:

MyStruct* mystruct_new(const char* filename) {
  MyStruct* ms = malloc(sizeof(MyStruct));
  if (!ms) {
    return NULL;
  }

  FILE* file = fopen(filename, "w+");
  if (!file) {
    free(ms);
    ms = NULL;
    return NULL;
  }

  ms->file = file;
  return ms;
}

main can look like this:

int main(void) {
  MyStruct* ms = mystruct_new("file.txt");
  if (!ms) {
    exit(EXIT_FAILURE);
  }

  // do something with `ms`...

  free_mystruct(ms);

  return EXIT_SUCCESS;
}

Solution

In this example, you are initializing a MyStruct variable with malloc(sizeof(MyStruct)). Thus, the free_mystruct code looks good as it is. It should safely free the structure created by mystruct_new.

It will first check to see if the argument MyStruct* s is null, if not then it will attempt to close the FILE* variable with fclose (if it exists), and finally it will free(s) which is the appropriate way to deallocate the memory created with malloc(sizeof(MyStruct)). It also sets the argument MyStruct* s to null, ~~which helps avoid dangerous use-after-free bugs that could appear in other parts of your code~~.

Regarding fclose failing

In some cases you won't need to worry about fclose failing because you are done with the file. fclose can fail for a variety of reasons, but your code as-is would not have memory leaks if fclose failed. This question is about creating and destroying instances of MyStruct, and the methods you've provided for doing so are reasonably safe (bugs are possible in code that uses this structure, but are not in the code you've provided).

UPDATE: Regarding 's = NULL'

In the comments there has been discussion regarding the utility of s = NULL as well as the potential of bugs such as 'use-after-free'.

Before getting into examples, it should be noted that in free_mystruct we are passing a MyStruct pointer by value. That means a new variable is created local to the function which stores the value of the pointer that was passed in. Changing the value of the local pointer does not change the value of the external one - however, changes to the address being pointed to will (which is why free(s) works).

Here is a simple example:

int main(void) {
  MyStruct* ms = mystruct_new("file.txt");
  if (!ms) {
    exit(1);
  }

  printf("The variable `ms` points to: %p\n", ms);
  printf("A few bytes at `ms`: %x\n", *ms);

  free_mystruct(ms);

  printf("The variable `ms` points to: %p\n", ms);
  printf("A few bytes at `ms`: %x\n", *ms);

  return 0;
}

When I run the above, I receive output such as the following:

The variable `ms` points to: 0x1516069f0
A few bytes at `ms`:         fca465a0
The variable `ms` points to: 0x1516069f0
A few bytes at `ms`:                0

This is what we should expect. The address of the pointer variable doesn't change, and therefore s = NULL does not take affect on that variable (outside of the free_mystruct) function. However, the data at ms appears to have been freed.

If you're following along this far, you should see that potential for a 'use-after-free' bug does still exist. It wasn't enough to nullify the pointer that was passed by value. It would be safer if the code did something like this:

free_mystruct(ms);
ms = 0;

Notice how the above sets the pointer to 0, but it is now the burden of the caller to do this (or ensure that the free'd MyStruct isn't referenced elsewhere in the code).

These types of nuances are common in C, where memory management is ultimately up to the developer. The code that the OP provided isn't buggy itself, but the developer using it should be aware of potential misuses that could result in security bugs.

DIGGING EVEN DEEPER

If we look another layer deeper we can see that free only nullifies the first byte of a dynamically allocated address range. That is why the output from earlier implied that the data at ms was null bytes after the free. In the following example, we can see that is not actually the case.

int main(void) {
  // STEP 1: Memory at MyStruct before malloc
  puts("STEP 1");
  MyStruct* ms;
  for (int i = 0; i < sizeof(MyStruct); i++)
    printf(" %02hhx", ms[i]);
  puts("\n");

  // STEP 2: Memory at MyStruct after malloc
  puts("STEP 2");
  ms = mystruct_new("file.txt");
  if (!ms) {
    exit(1);
  }
  printf("The variable `ms` points to: %p\n", ms);
  printf("A few bytes at `ms`");
  for (int i = 0; i < sizeof(MyStruct); i++)
    printf(" %02hhx", ms[i]);
  puts("\n");

  // STEP 3: Memory at MyStruct after free
  puts("STEP 3");
  free_mystruct(ms);
  printf("The variable `ms` points to: %p\n", ms);
  printf("A few bytes at `ms`");
  for (int i = 0; i < sizeof(MyStruct); i++)
    printf(" %02hhx", ms[i]);
  puts("\n");

  return 0;
}

Example output:

STEP 1
 88 00 80 00 01 01 00 80

STEP 2
The variable `ms` points to: 0x14ce069f0
A few bytes at `ms` a0 00 00 00 03 00 00 00

STEP 3
The variable `ms` points to: 0x14ce069f0
A few bytes at `ms` 00 00 60 00 03 00 00 00

The main takeaway here is that free(...) does not nullify all of the bytes in the memory that was de-allocated. That means that the data stored at a dynamically allocated location may remain in memory after free(...) is called.

This is yet another reason why not setting ms = NULL after deallocating the structure could be dangerous.