Wrap C block in function. Beginner question

Question

I found the C snippet to get the current working directory from here. Essentially, the code is:

char directory[_MAX_PATH];
getcwd(directory, sizeof(directory))

I want to abstract that into another function, in a different file (so it can be swapped out on different platforms if necessary).

Currently, I have in the external file

void getCurrentDirectory(char *directory) {
    getcwd(directory, sizeof(directory));
}

and in the main file

char directory[100];
getCurrentDirectory(directory);
printf("%s", *directory);

However, when printing to screen, I get nonsense (possibly trying to print memory location as a string?)

I'm sure it's something blindingly obvious to a non-beginner. What's going on?

Edit: I'm on Windows 7, btw

Thanks.

True. It's not. It will form part of a C++ program eventually. Thanks — Adam, May 23 '11 at 16:01
@jv42 I guess it is because C++ is a superset of C (more or less), no? — Adam, May 23 '11 at 16:10
@All Let's not start the debate again, this is using C lib functions, C style strings, C style console output, this is C. There are C++ ways of doing the same things. — jv42, May 23 '11 at 16:13
@jv42: So he's doing C++ wrong. Our duty then is to correct his C++. — Benjamin Lindley, May 23 '11 at 16:16
@Benjamin that's somewhat beyond the scope of this question, is it not? What if someone was looking for the C way to do things, they come here, and all they see is "you're doing it wrong, use C++ properly instead"? — badgerr, May 23 '11 at 16:56
@badgerr: But he wasn't asking that. He asked about C++. Adam put the C++ tag on there, then jv42 removed it and replaced it with a C tag, which would prevent many people from even seeing the question who might be able to give him a better C++ solution. — Benjamin Lindley, May 23 '11 at 17:02
@Benjamin fair enough. I just noticed the subject says C, and he refers to a "C snippet". That's enough to suggest it might be a C question I suppose. — badgerr, May 23 '11 at 17:04

badgerr · Answer 1 · 2011-05-23T16:53:47.297

2

You are passing the size of a char* to getcwd, instead of the size of the array.

Pass a size parameter to your function.

void getCurrentDirectory(char *directory, size_t size) {
    getcwd(directory, size);
}

and then:

char directory[100];
getCurrentDirectory(directory, sizeof(directory));
printf("%s", *directory);

Also, if you're using Windows, you should probably change your array size to the predefined MAX_PATH to avoid a potential buffer overflow. getcwd takes a length, but I don't think all of the file functions do.

edited May 23 '11 at 16:53

answered May 23 '11 at 15:57

badgerr

7,802
2
28
43

That works, only if I remove the * from the printf parameter. Also, where is MAX_PATH defined? – Adam May 23 '11 at 16:06
Sorry, it's in limits.h, but it's not really necessary for getcwd, as you give it a max length as a parameter already. Sorry for the confusion. – badgerr May 23 '11 at 16:50

score 2 · Answer 2 · answered May 23 '11 at 15:58

2

This line: printf("%s", *directory);

should be: printf("%s", directory);

You're passing in the first element (directory[0]) to the printf, not the pointer to the char array.

answered May 23 '11 at 15:58

JosephH

8,465
4
34
62

score 2 · Answer 3 · edited May 23 '17 at 10:34

2

If it's C++ I'd suggest using boost::filesystem if at all possible, which hides all of the underlying platform details and gives you C++ style interface instead of the buffer overflow prone C functions.

edited May 23 '17 at 10:34

Community

1
1

answered May 23 '11 at 15:58

Flexo

87,323
22
191
272

Thanks, will look into that. I've not got so far as figuring out properly how C++ does OOP yet, especially interfaces. – Adam May 23 '11 at 16:07
My general guideline for any C++ programing would be "prefer the C++ way". `getcwd`, `printf` and `char` strings instead of `std::string` are all contrary to that principle (and hence the "is this C++" discussion taking place on the question comments) – Flexo May 23 '11 at 16:16

Alok Save · Accepted Answer · 2011-05-23T16:07:05.833

Theres a number of things that you are doing wrong here:

void getCurrentDirectory(char *directory) 
  {
      getcwd(directory, sizeof(directory));
  }

Mistake 1:

`sizeof(directory)`

gives you size of a pointer, to be precise, char *. Your intention is to pass size of the array, and not the pointer size.

Mistake 2:

`printf("%s", *directory);`

Passes first element of the array to the printf not the address of the array. Your intention is to print the entire array not just the first element.

Corrected Solution

You should be doing

void getCurrentDirectory(char *directory, size_t arrSize)  
{                                         ^^^^^^^^^^^^^^
    getcwd(directory, arrSize);
}

Size of the array is passed explicitly so the function can just use it.

In the main while printing the contents of the array:

   printf("%s", directory);

That's the conclusion I've come to based on badgerr's answer. Without correcting #2, I got a segfault. Thanks — Adam, May 23 '11 at 16:13

score 1 · Answer 5 · answered May 23 '11 at 16:10

You should be allocating the buffer locally (where the necessary length is known, and the actual length needs to be known) and returning a string:

std::string
getCurrentDirectory()
{
    char results[_MAX_PATH];
    if ( getcwd( results, sizeof(results) ) == NULL )
        throw std::ios_base::failure( "Could not get current directory" );
    return std::string( results );
}

Note too that _MAX_PATH is only a guess; the actual maximum is not a compile time constant (since it depends on the file system). An implementation which takes this into account might look something like:

std::string
getCurrentDirectory()
{
    long length = pathconf( ".", _PC_PATH_MAX );
    if ( length == -1 )
        throw std::ios_base::failure(
                "Could not determine necessary buffer length to get current directory" );
    std::string results( length, '\0' );
    if ( getcwd( &results[0], results.size() ) == NULL )
        throw std::ios_base::failure( "Could not get current directory" );
    results.resize( strlen( results.c_str() );
    return results;
}

This is probably overkill, however, if the program is only going to be used on a personal system with no NFS or SMB mounted drives.

Benjamin Lindley · Answer 6 · 2011-05-23T17:36:40.700

1

Since it is C++, why not do this:

std::string getCurrentDirectory()
{
    char directory[_MAX_PATH] = {};
    getcwd(directory, sizeof(directory));
    return directory;
}

edited May 23 '11 at 17:36

answered May 23 '11 at 16:10

Benjamin Lindley

101,917
9
204
274

score 0 · Answer 7 · answered May 23 '11 at 15:57

You cannot find out the size of a memory block pointed at by a pointer using sizeof like that. It will evaluate to the size of the pointer itself.

Change your function to:

void getCurrentDirectory(char *directory, size_t buf_max)
{
    getcwd(directory, buf_max);
}

score 0 · Answer 8 · answered May 23 '11 at 16:19

Now to answer what you've asked:

When getcwd fails for some reason the contents of the array pointed to by directory (in your case) is undefined. Therefore, with your buggy implementation you will see junk in most cases. (Also, you should check return value from getcwd, it returns -1 when it fails)

Now, the reason for failure in your case is the size that you are specifying using sizeof(directory) is just the size of a pointer (which would likely to be 4) and characters in the name of current working directory you are trying to print are more than that. This will work fine for directory of size 3 or less.

And, finally many others here have already explained you how to possibly fix it.

Wrap C block in function. Beginner question

8 Answers8