How to return a string from GetOpenFileNameA

Question

This is a function to open a file dialog in Windows and return a string with the file name:

#include <windows.h>
#include <commdlg.h>
#include <string.h>
char* openFileDlg(char FileTypes[]);
char* openFileDlg(char FileTypes[]){
    OPENFILENAME ofn;
    char szFile[260];
    HWND hwnd;
    HANDLE hf;
    ZeroMemory(&ofn, sizeof(ofn));
    ofn.lStructSize = sizeof(ofn);
    ofn.hwndOwner = hwnd;
    ofn.lpstrFile = szFile;
    ofn.lpstrFile[0] = '\0';
    ofn.nMaxFile = sizeof(szFile);
    strcpy(ofn.lpstrFilter,FileTypes);
    ofn.nFilterIndex = 1;
    ofn.lpstrFileTitle = NULL;
    ofn.nMaxFileTitle = 0;
    ofn.lpstrInitialDir = NULL;
    ofn.Flags = OFN_PATHMUSTEXIST | OFN_FILEMUSTEXIST;
    if(GetOpenFileNameA(&ofn)){
        char *toReturn;
        sprintf(toReturn,"%s",ofn.lpstrFile);
        return toReturn;
    }
    else{
        return NULL;
    }
}

When I call this function and open a file, the process ends and returns value 3 (which means there is an error). How can I do so that this function returns a string with the path of the selected file?

Edit: I've changed my code to this and it still doesn't work:

#include <windows.h>
#include <commdlg.h>
#include <string.h>
void openFileDlg(char *toReturn[],char FileTypes[]);
void openFileDlg(char *toReturn[],char FileTypes[]){
    OPENFILENAME ofn;
    /*
    Code for the settings of the GetOpenFileNameA, irrelevant in this question.
    If you really need to know what's here, look at the code above.
    */
    if(GetOpenFileNameA(&ofn)){
        strcpy(*toReturn,ofn.lpstrFile);
    }
    else{
        sprintf(*toReturn,"");
    }
}

I should also say that if I press the Cancel button in the open file dialog box instead of selecting a file, it works fine. After some tests, I've noticed that it's the line strcpy(*toReturn,ofn.lpstrFile); that causes the error.

If GetOpenFileNameA() makes your program terminate then you need to get your machine fixed. Start with the custom shell extensions, ask for help at superuser.com — Hans Passant, Jul 11 '16 at 14:27
strcpy(ofn.lpstrFilter,FileTypes); here you attempt to write to null address but there are many more errors — David Heffernan, Jul 11 '16 at 14:35
`ofn.hwndOwner = hwnd;` hwnd is uninitialized, do just `ofn.hwndOwner = NULL;`. — Sergio, Jul 11 '16 at 14:49
`GetOpenFileNameA` limits your users to opening files, that can be represented using the current codepage only. This feels like a rather arbitrary decision. I would suggest using `GetOpenFileNameW` instead, that can deal with any filename. — IInspectable, Jul 11 '16 at 15:22
@IInspectable I tried that and the part where you can select file types is now written in Japanese or Chinese or some Asian language. Anyway, it didn't solve my problem. — Donald Duck, Jul 11 '16 at 15:29
@DonaldDuck You are giving up to soon. The Chinese you see is because your code is wrong. The cast you added which lied to the compiler is responsible. Don't give up when you make mistakes, work out what you did wrong, and do it right. A great many mistakes here. You are going to need to spend quite a bit of time learning. If you don't recognise that you are just going to carry on making these mistakes for a long time. — David Heffernan, Jul 11 '16 at 15:56
@DavidHeffernan OK, but that doesn't answer my question: What's the problem with the `strcpy(*toReturn,ofn.lpstrFile);` line? — Donald Duck, Jul 11 '16 at 16:01
Why are you passing an array of `char*`? You mean to pass an array of `char`. So `char toReturn[]` rather than `char *toReturn[]`. There are still a great many errors. You have an surplus of self-confidence. You aren't as competent as you think you are. Until you recognise that you don't understand any of the code that you have written above you won't be able to make progress. That sounds harsh I know. If you can't take criticism you'll just ignore me and get upset, and you'll continue to be lacking the required knowledge. — David Heffernan, Jul 11 '16 at 16:05
@DavidHeffernan `char *toReturn[]` is a pointer to a string because to call the function, I use `char result[100], openFileDlg(result,"All files (*.*)\0*.*\0");` so that `result` contains the string that I want. If I use `char toReturn[]` it will change `toReturn` inside the `openFileDlg` function but it won't change `result` so when the function is done all the changes will be lost. That's why I'm passing an array of `char*`. If that's not the way to define a pointer which points at a string, I would be glad if you tell me how to do. — Donald Duck, Jul 11 '16 at 16:15
Never mind. You seem to know more about this than I do. If you are sure that your parameter type is correct then I won't argue with you. — David Heffernan, Jul 11 '16 at 16:18

score 5 · Answer 1 · answered Jul 11 '16 at 14:24

The pointer variable toReturn doesn't point anywhere, using it in any way without initializing it (i.e. making it point somewhere valid and big enough) will lead to undefined behavior

You have two solutions really:

Allocate memory dynamically and return a pointer to that. This of course requires the caller to free the memory when done with it.
Have the function take another two arguments: A pointer to a buffer and the length of the buffer. Then copy the string into that buffer, and return a boolean "true" or "false" success/failure status.

I recommend solution number two.

On an unrelated note, there's no need to use the expensive sprintf function in your case, a simple strcpy (or strncpy if you go with the second solution) will do.

You also have to remember in both cases that strings in C have an actual length of one more than e.g. strlen reports, for the terminating '\0' character.

Joris · Answer 2 · 2016-07-11T15:02:16.767

In general, if you want to return a string in C, I'd use one of the following methods:

1) pass in a string buffer for the method to write to:

int openFileDlg(char FileTypes[], char* toReturn, int bufLen) {
    /* ... */
    snprintf(toReturn, bufLen, /* what you want to print */);
    return ERROR; // status-code
} 

/* ... */

char errorBuf[80];
int result;

result = openFileDlg(..., errorBuf, sizeof(errorBuf));

2) allocate memory, expect caller to free it:

char* openFileDlg(char FileTypes[]) {
    /* ... */
    char *toReturn = malloc(/* big enough */);
    sprintf(toReturn, /* what you want to print */);
    return toReturn;
} 

/* ... */
char* error = openFileDlg(...);
if (error) {
    /* ... */
    free(error);
}

personally, I'd prefer (1) because it's safer. Option (2) is nicer to the API of the function, but has a risk of memory leaks if you forget to free the returned buffer. In a bigger project (especially with multiple people working on it) this is a very real risk.

(I realise this is pretty much the same as Joachim's answer, but his went up as I was writing mine)

score 1 · Answer 3 · answered Jul 11 '16 at 14:25

You did not allocate memory for your return value. If you know the length of ofn.lpstrFile you could do this:

    char *toReturn = malloc( (sizeOfLpstrFile + 1) * sizeof(char)) ;
    sprintf(toReturn,"%s",ofn.lpstrFile);
    return toReturn;

Still I consider this a bad idea because the calling function will have to free the memory which is not obvious from the interface.

How to return a string from GetOpenFileNameA

3 Answers3