0

I am trying to create a directory with log entries with mkstemp. But, to my understanding I mustn't pass a string constant to mkstemp. I allocate memory for the string and use snprintf to format the output which I thought was going to work but mkstemp returns a negative value setting errno to EINVAL.

However, in the linux manual for mkstemp it clearly says:

EINVAL For mkstemp() and mkostemp(): The last six characters of template were not XXXXXX; now template is unchanged.

Furhtermore mkstemp never modifies my dynamic string.

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

#define LOG_DIR "/tmp"

int main(int argc, char **argv) {
  char *fname;
  FILE *fp;

  if(argc != 3) {
    fprintf(stderr, "Usage: %s <msg> <severity>\n", argv[0]);
    return 0;
  }

  int length = snprintf(NULL, 0, "%s/log_entry.XXXXXX", LOG_DIR); // snprintf returns the required length for my string
  if(length < 0) {
    perror("snprintf failed");
    return 1;
  }
  fname = malloc(sizeof(char) * length); // allocate memory for fname based on the return value of snprintf
  snprintf(fname, length, "%s/log_entry.XXXXXX", LOG_DIR); // send formatted output into fname

  int fd = mkstemp(fname); // this returns -1 and errno is set to 22
  if(fd < 0) {
    perror("failed to create entry file");
    return 1;
  }
  fp = fdopen(fd, "w");
  if(fp == NULL) {
    perror("failed to open entry file");
    return 1;
  }
  fprintf(fp, "\"%s\" %d ",argv[1], atoi(argv[2]));
  fflush(fp);
  fclose(fp);
  free(fname);
  return 0;
}

This snippet spits out an error on both of my Linux machines however if I remove the dynamic allocated string and explicitly set fname it works

char fname[] = "/tmp/log_entry.XXXXXX";
Linus
  • 1,516
  • 17
  • 35
  • 2
    Looks like you forgot to allocate the null-terminating character! Also, I recommend using `asprintf` for your usage case. It's one call rather than two. – Noldorin Oct 24 '15 at 13:26
  • @Noldorin `fname = malloc(sizeof(char) * length + 1);` doesn't work if that's what you mean. – Linus Oct 24 '15 at 13:26
  • Add `1` where you set the `length` variable. That should do the job. If not, what does `snprintf` return? – Noldorin Oct 24 '15 at 13:28
  • @Noldorin `fname = malloc(sizeof(char) * length+1); snprintf(fname, length+1, "%s/log_entry.XXXXXX", LOG_DIR);` still gives me the same error. – Linus Oct 24 '15 at 13:30
  • 1
    @Linus What error ? Post it . – ameyCU Oct 24 '15 at 13:31
  • Right, tell us the error! The return value of `snprintf` and what `perror` outputs. – Noldorin Oct 24 '15 at 13:33
  • @ameyCU Never mind, it works now for some reason. A follow up question, why does `snprintf(fname, length+1, "%s/log_entry.XXXXXX", LOG_DIR);` still work? If I don't add one to malloc? – Linus Oct 24 '15 at 13:34
  • Also, it logically makes sense to put the brackets around (length + 1) even though it probably won't make a difference on your system, since `sizeof (char)` is probably `1. – Noldorin Oct 24 '15 at 13:34
  • @Linus Compiler wont stop you to access invalid memory , if you are lucky you get seg fault . But still that is undefined behaviour. – ameyCU Oct 24 '15 at 13:35
  • @Linus Because you're getting lucky, and the character after the malloc'd block happens to be readable, and starts with a `0`. Don't do it though, it's unsafe. – Noldorin Oct 24 '15 at 13:35
  • Okay thanks guys. I swear I tried adding one to malloc before, but I must've forgot to add one to `snprntf`. – Linus Oct 24 '15 at 13:36
  • 1
    Sure. But like I said, save yourself all this headache and just use `asprintf`. ;) – Noldorin Oct 24 '15 at 13:37
  • @Noldorin Right, I'm looking into it right now. Sounds useful :) – Linus Oct 24 '15 at 13:38
  • @Linus http://man7.org/linux/man-pages/man3/asprintf.3.html – ameyCU Oct 24 '15 at 13:39
  • Just to note that `asprintf` is neither C standard nor does it appear to be POSIX. – too honest for this site Oct 24 '15 at 14:24

2 Answers2

2
fname = malloc(sizeof(char) * length);

should be:

fname = malloc(sizeof(char) * (length + 1));

Now

snprintf(fname, length+1, "%s/log_entry.XXXXXX", LOG_DIR);

will create the filename. In your version, the filename did not end with 6 'X's which made mkstemp fail.

rici
  • 234,347
  • 28
  • 237
  • 341
Paul Ogilvie
  • 25,048
  • 4
  • 23
  • 41
  • Also: in OP's version, there was a NUL termnator but only 5 Xs. snprintf truncates to allow space for the terminator. – rici Oct 24 '15 at 13:44
  • @rici, fixed your first comment. From VC2013 documentation: "_If len = count, then len characters are stored in buffer, no null-terminator is appended, and len is returned._" so no null would be appended. – Paul Ogilvie Oct 24 '15 at 14:15
  • MSVC is not Posix, of course, but the deviations are disappointing. See http://pubs.opengroup.org/onlinepubs/9699919799/functions/fprintf.html "Otherwise, output bytes beyond the n-1st shall be discarded instead of being written to the array, and a null byte is written at the end of the bytes actually written into the array." – rici Oct 24 '15 at 14:34
  • Since OP specifically refers to Linux, we can assume the Posix-compliant glibc implementation, which null-terminates unless size is 0. http://man7.org/linux/man-pages/man3/sprintf.3.html – rici Oct 24 '15 at 14:38
  • And finally, the sentence you quote is for `_snprintf`. As of 2015, the MS CRT finally supports C99 `snprintf`, apparently correctly. See http://blogs.msdn.com/b/vcblog/archive/2014/06/10/the-great-crt-refactoring.aspx?PageIndex=2 search for the comment about the snprintf variants – rici Oct 24 '15 at 15:05
  • @rici, that s a great improvement in C99. Thanks for pointing it out to me. Then, in the OP's case, the file would be created, only wth a different name. But the file wasn't created, probably beause it had invalid characters in it (`errno` 22 = `EINVAL`), which can only be explained by no `\0` with invalid characters in the memory following the allocated memory. What's your opinion? – Paul Ogilvie Oct 24 '15 at 15:19
  • My opinion is that the filename created by snprintf ended with 5 Xs. The documentation for mkstemp clearly states that EINVAL will be returned if the filename does not end with six Xs. – rici Oct 24 '15 at 15:45
  • @rici, thanks. Solid explanation. I updated the solution with this info. – Paul Ogilvie Oct 24 '15 at 17:33
1
 fname = malloc(sizeof(char) * length);

You fill it completely , leave no space for '\0' . :eave a space for null terminator -

 fname = malloc(sizeof(char) *(length+1));

Then increase length in snprintf also to length+1 .

ameyCU
  • 16,489
  • 2
  • 26
  • 41