Compiler warning: use of mktemp is dangerous. Fixes #956

[pmqs]

Silence the compiler warning below by using mkdtemp instead of mktemp

57%] Linking C executable minigzip
/usr/bin/ld: libminizip.a(mz_os_posix.c.o): in function `mz_os_get_temp_path':
/home/runner/work/minizip-ng/minizip-ng/mz_os_posix.c:374:(.text+0x1023): warning: the use of `mktemp' is dangerous, better use `mkstemp' or `mkdtemp'
[ 57%] Built target minigzip_cli
[ 60%] Building C object CMakeFiles/minizip_cli.dir/minizip.c.o
[ 62%] Linking C executable minizip
/usr/bin/ld: libminizip.a(mz_os_posix.c.o): in function `mz_os_get_temp_path':
/home/runner/work/minizip-ng/minizip-ng/mz_os_posix.c:374:(.text+0x1023): warning: the use of `mktemp' is dangerous, better use `mkstemp' or `mkdtemp'

[nmoinvaz]

The filename is f?

[pmqs]

The filename is f?

Yes.

[tbeu]

Suggested change

	/* Check for no environment variable set at all */
	if (!tmp_dir)
	return MZ_INTERNAL_ERROR;

Is this reachable actually?

[pmqs]

Good catch. I missed the last if statement in the block of code before this. I'll update it

    if (!tmp_dir)
        tmp_dir = "/tmp";

[nmoinvaz]

Maybe I am a dumb-dumb, but why is random dir with a fixed name more secure than a random filename with a fixed dir?

[pmqs]

Maybe I am a dumb-dumb, but why is random dir with a fixed name more secure than a random filename with a fixed dir?

I believe that mktemp is deprecated because it doesn't create a the temporary file in the filesystem, so there is a potential race condition where a bad actor guesses the filename before you get a change to use it.

mkdtemp atomically creates the directory, so no race condition.

[Coeur]

BUGS

  Never use mktemp().  Some implementations follow 4.3BSD and
  replace XXXXXX by the current process ID and a single letter, so
  that at most 26 different names can be returned.  Since on the one
  hand the names are easy to guess, and on the other hand there is a
  race between testing whether the name exists and opening the file,
  every use of mktemp() is a security risk.  The race is avoided by
  [mkstemp(3)](https://man7.org/linux/man-pages/man3/mkstemp.3.html) and [mkdtemp(3)](https://man7.org/linux/man-pages/man3/mkdtemp.3.html).

Source: https://man7.org/linux/man-pages/man3/mktemp.3.html

[nmoinvaz]

@Coeur thanks that makes sense.

I suggest instead of f for filename we use snprintf with just the unix timestamp.

[pmqs]

@Coeur thanks that makes sense.

I suggest instead of f for filename we use snprintf with just the unix timestamp.

Done

[nmoinvaz]

Sorry for this, but I think this is a newer C standard thing right? Maybe just best to do calloc().

[tbeu]

C99 brought stack-allocated variable-length arrays, see https://en.wikipedia.org/wiki/Variable-length_array#C.

Yes, better stick with heap-allocated array here.

[pmqs]

Ha! I knew that zlib-ng policed this one in cmake and assumed minizip-ng did as well.

I've made the change, but can we get cmake to police a minimum version?

[nmoinvaz]

char *temp_path = NULL;

[nmoinvaz]

temp_path = (char *)calloc(max_path, sizeof(char));

Don't forget to do free(temp_path) also for every failure path after.

[pmqs]

temp_path = (char *)calloc(max_path, sizeof(char));

Don't forget to do free(temp_path) also for every failure path after.

Wow! I run with address sanitizer enabled all the time to catch things like this.

Hmm, just looking at the compiler commandline I'm not seeing the expected options.

Anyway, I've added the free

[nmoinvaz]

This is still not fixed, there are return paths where it would leak memory.

[pmqs]

I'm on it.