Fix file leak on error path

[jesferman]

Close file when mpack_tree_init_stdfile() fails after checking its size

Leak uncovered by gcc. Full trace:

mpack.c:6098:9: error: leak of FILE ‘file’ [CWE-775] [-Werror=analyzer-file-leak]
 6098 |         return;
      |         ^~~~~~
  ‘mpack_tree_init_filename’: events 1-8
    |
    | 6106 | void mpack_tree_init_filename(mpack_tree_t* tree, const char* filename, size_t max_bytes) {
    |      |      ^~~~~~~~~~~~~~~~~~~~~~~~
    |      |      |
    |      |      (1) entry to ‘mpack_tree_init_filename’
    | 6107 |     if (!mpack_tree_file_check_max_bytes(tree, max_bytes))
    |      |        ~
    |      |        |
    |      |        (2) following ‘false’ branch...
    |......
    | 6111 |     FILE* file = fopen(filename, "rb");
    |      |                  ~~~~~~~~~~~~~~~~~~~~~
    |      |                  |
    |      |                  (3) ...to here
    |      |                  (4) opened here
    | 6112 |     if (!file) {
    |      |        ~
    |      |        |
    |      |        (5) assuming ‘file’ is non-NULL
    |      |        (6) following ‘false’ branch (when ‘file’ is non-NULL)...
    |......
    | 6117 |     mpack_tree_init_stdfile(tree, file, max_bytes, true);
    |      |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |     |
    |      |     (7) ...to here
    |      |     (8) calling ‘mpack_tree_init_stdfile’ from ‘mpack_tree_init_filename’
    |
    +--> ‘mpack_tree_init_stdfile’: events 9-11
           |
           | 6096 | void mpack_tree_init_stdfile(mpack_tree_t* tree, FILE* stdfile, size_t max_bytes, bool close_when_done) {
           |      |      ^~~~~~~~~~~~~~~~~~~~~~~
           |      |      |
           |      |      (9) entry to ‘mpack_tree_init_stdfile’
           | 6097 |     if (!mpack_tree_file_check_max_bytes(tree, max_bytes))
           |      |        ~
           |      |        |
           |      |        (10) following ‘true’ branch...
           | 6098 |         return;
           |      |         ~~~~~~
           |      |         |
           |      |         (11) ...to here
           |
    <------+
    |
  ‘mpack_tree_init_filename’: event 12
    |
    | 6117 |     mpack_tree_init_stdfile(tree, file, max_bytes, true);
    |      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |     |
    |      |     (12) returning to ‘mpack_tree_init_filename’ from ‘mpack_tree_init_stdfile’
    |
  ‘mpack_tree_init_stdfile’: event 13
    |
    | 6098 |         return;
    |      |         ^~~~~~
    |      |         |
    |      |         (13) ‘file’ leaks here; was opened at (4)
    |
cc1: all warnings being treated as errors

[ludocode]

Thanks. This fix is fine. The documentation for this function states that the file is closed (if requested) before the function is returned regardless of errors, so yes, we should close the file here.

It's worth pointing out that this particular error can only happen if an invalid value was passed to max_bytes. In a debug build MPack will actually break in the debugger if this happens, while in release it raises mpack_error_bug and doesn't parse at all, so it's not like a bad value here would cause a leak to go unnoticed. The value for max_bytes is probably given as a compile-time constant so the odds of this causing a real leak are remote. It would have to be computed to some huge value larger than LONG_MAX but still fitting in the unsigned size_t, and on most UNIX-like systems those types are the same width.

A better fix might be to remove this check and the mpack_tree_file_check_max_bytes() function entirely. I'm not really sure why it's there. The code in mpack_file_tree_read() seems like it would handle large values of max_bytes fine (though it could be simplified a bit.) We could also improve the code to query the file size so that ftell() is only a fallback; we could use ftello() or fstat() where available. This would also allow it to work on messages larger than 2 GiB (not that I think there's much point in supporting such huge messages, but it's generally good to eliminate any surprising or unexpected limits.) At the very least the LONG_MAX limit needs to be well documented, and/or should not break in the debugger nor raise mpack_error_bug.