Pass by address, scanf, de-referencing

Question

I have written a program employing the following concept:

I create an integer x pass by address to a function, along with a filename, said function opens file if available, scans the first line and sets the value that pX points to equal to the scanned line.

Only it's not working, and I don't see what I'm doing wrong. As far as I can tell the code below is generally how one would accomplish it, but I'm not sure if I'm not using scanf() correctly with the pointer or what.

void foo() {
    char input[20] = "test.txt";
    int x = 1;
    bar(input, &x);
}

void bar(char *fileName, int *pX) {
    FILE *fp = fopen(fileName, "r");
    char *buffer = malloc(15 * sizeof(int));
    fgets(buffer, 15, fp);
    scanf(buffer, "%d", *pX);
    free(buffer);
    fclose(fp);
}

The `scanf` function wants pointers for the arguments. `pX` is a pointer. `*pX` is *not* a pointer. — Some programmer dude, Mar 06 '17 at 07:35
`15 * sizeof(int)` You're allocating more size than what you need. You probably wanted `15 * sizeof(char)` which is the same as `15` as `sizeof(char)` is equal to 1 — Spikatrix, Mar 06 '17 at 07:47
Please please turn up your compiler warning level (`-Wall -Wextra` for *gcc* and *clang*). If your compiler does not provide proper warnings, please switch compiler. Then fix the warnings you get (ask here if you don't understand them or don't know how to fix, it's easy to write a good quality question for something that straightforward). — hyde, Mar 06 '17 at 08:01

score 2 · Accepted Answer · edited May 23 '17 at 11:53

2

Change line :

scanf(buffer, "%d", *pX);

to :

sscanf(buffer, "%d", pX);

You need function sscanf for what you are trying to do.

Both scanf and sscanf take a pointer as an argument. pX is of type int *, therefore a pointer to an int and should work for you. What you pass, *pX, is the contents of this pointer, in other words an int.

Also, change line :

char *buffer = malloc(15 * sizeof(int));

to :

char *buffer = malloc(15 * sizeof(char));

or simply :

char *buffer = malloc(15);

and always check the result of malloc :

if (buffer == NULL){
    ...
}

edited May 23 '17 at 11:53

Community

1
1

answered Mar 06 '17 at 07:36

Marievi

4,951
1
16
33

Shouldn't it be : scanf(buffer, "%d", pX); ?? – scorpGoku Mar 06 '17 at 07:36
`scanf(buffer, "%d", pX);` are you sure this is even valid? – Sourav Ghosh Mar 06 '17 at 07:42
1

thank you. sscanf(buffer, "%d", pX); solved my issue. – Mar 06 '17 at 07:48
IMO, `buffer = malloc(sizeof *buffer * 15);` is even better for allocations. No need to code the matching type. – chux - Reinstate Monica Mar 06 '17 at 15:20

Sourav Ghosh · Answer 2 · 2017-03-06T07:50:16.593

First of all, there's no pass-by-reference in C, function parameters are all passed by value. By passing a pointer-to-data, we make the simulation of the achieving the same effect as pass-by-reference, but that does not mean C has any pass-by-reference concept.

That said, the problem seems to be

 scanf(buffer, "%d", *pX);
                     ^^^^

where

the current syntax is invalid and invokes undefined behavior. Probably you need sscanf().
px is already a pointer to int. Passing px will be correct and suffice.

Moral of the story: enable compiler warnings and pay heed to them. They are there for a reason. With proper warnings enabled, you should see something like

warning: format %d expects argument of type int *, but argument 3 has type int [-Wformat=]

Finally,

Always check the return value of fopen() for success before using the file pointer.
Check for the return value of scanf() to ensure successful scanning.
Check for the return value of fgets() to ensure success

...basically, check return value of all library calls to make sure they worked as expected.

I've tried this. It doesn't seem to work. At least in the actual program I'm writing. — , Mar 06 '17 at 07:40
@wanderbread If you already know, great!! Let's allow it to be there for whom might be a little more confused, shall we? — Sourav Ghosh, Mar 06 '17 at 14:41

chqrlie · Answer 3 · 2017-03-06T07:55:33.690

You use scanf() incorrectly: either use scanf directly to parse standard input or use sscanf() to parse the string read by fgets(). Furthermore, pX is already a pointer to int, which is what sscanf() expects to store the int value it converts, pass it directly: sscanf(buffer, "%d", pX);

Here is a modified version:

int bar(const char *fileName, int *pX) {
    char buffer[15];
    FILE *fp = fopen(fileName, "r");
    int success = 0;

    if (fp != NULL) {
        fgets(buffer, sizeof buffer, fp);
        if (sscanf(buffer, "%d", pX) == 1)
            success = 1;
        fclose(fp);
    }
    return success;
}

void foo(void) {
    int x = 1;
    bar("test.txt", &x);
    /* do something with x */
}

Notes:

allocating buf is unnecessary, just make it a local array with automatic storage.
char *buffer = malloc(15 * sizeof(int)); is incorrect: you allocate space for 15 int instead of 15 characters, which have a size of 1 by definition. Use the size of the destination type to avoid any inconsistencies:
```
char *buffer = malloc(15 * sizeof(*buffer));
```
always check the return value of malloc() to avoid potential undefined behavior.
reading from fp without checking if fopen succeeded has potential undefined behavior.
the contents of the array pointed to by filename is not modified, make it a const char *.
it may be useful for bar to return a success indicator.
enable more warnings at compile time: gcc -Wall -Wextra -Werror or clang -Weverything -Werror might have caught the mistakes in scanf.

What do you mean be automatic storage? In my actual application I need to be able to handle potentially very large files. Allocation based on the whole file size seemed redundant but an effective way to make sure that was possible. — , Mar 06 '17 at 12:56
Also thank you for the suggestion of including the "-Weverything" and "-Werror" arguments. This is great, and will help immensely in the future. Is there anyway I can turn this options on permanently? — , Mar 06 '17 at 14:26
@wanderbread: automatic storage is the default for local variables in function scope, as opposed to static storage with the `static` keyword. A small buffer, up to a few kilobytes are OK for automatic storage, larger arrays should be allocated from the heap and freed after use. — chqrlie, Mar 06 '17 at 16:54

Pass by address, scanf, de-referencing

3 Answers3