3

I have an application that tries to read a specific key file and this can happen multiple times during the program's lifespan. Here is the function for reading the file:

__status 
_read_key_file(const char * file, char ** buffer)
{
    FILE * pFile = NULL;
    long fsize = 0;

    pFile = fopen(file, "rb");
    if (pFile == NULL) {
        _set_error("Could not open file: ", 1);
        return _ERROR;
    }

    // Get the filesize
    while(fgetc(pFile) != EOF) {
        ++fsize;
    }

    *buffer = (char *) malloc(sizeof(char) * (fsize + 1));

    // Read the file and write it to the buffer
    rewind(pFile);

    size_t result = fread(*buffer, sizeof(char), fsize, pFile);

    if (result != fsize) {
        _set_error("Reading error", 0);
        fclose(pFile);
        return _ERROR;
    }

    fclose(pFile);
    pFile = NULL;

    return _OK;
}

Now the problem is that for a single open/read/close it works just fine, except when I run the function the second time - it will always segfault at this line: while(fgetc(pFile) != EOF)

Tracing with gdb, it shows that the segfault occurs deeper within the fgetc function itself. I am a bit lost, but obviously am doing something wrong, since if I try to tell the size with fseek/ftell, I always get a 0.

Some context:

  • Language: C
  • System: Linux (Ubuntu 16 64bit)
  • Please ignore functions and names with underscores as they are defined somewhere else in the code.
  • Program is designed to run as a dynamic library to load in Python via ctypes

EDIT Right, it seems there's more than meets the eye. Jean-François Fabre spawned an idea that I tested and it worked, however I am still confused to why.

Some additional context:

Suppose there's a function in C that looks something like this:

_status 
init(_conn_params cp) {

    _status status = _NONE;

    if (!cp.pkey_data) {
        _set_error("No data, open the file", 0);

        if(!cp.pkey_file) {
            _set_error("No public key set", 0);
            return _ERROR;
        }

        status = _read_key_file(cp.pkey_file, &cp.pkey_data);
        if (status != _OK) return status;
    }

    /* SOME ADDITIONAL WORK AND CHECKING DONE HERE */

    return status;
}

Now in Python (using 3.5 for testing), we generate those conn_params and then call the init function:

from ctypes import *

libCtest = CDLL('./lib/lib.so')

class _conn_params(Structure):
    _fields_ = [
        # Some params

        ('pkey_file', c_char_p),
        ('pkey_data', c_char_p),

        # Some additonal params
    ]

#################### PART START #################
cp = _conn_params()
cp.pkey_file = "public_key.pem".encode('utf-8')

status = libCtest.init(cp)
status = libCtest.init(cp) # Will cause a segfault
##################### PART END ###################

# However if we do

#################### PART START #################
cp = _conn_params()
cp.pkey_file = "public_key.pem".encode('utf-8')
status = libCtest.init(cp)

# And then

cp = _conn_params()
cp.pkey_file = "public_key.pem".encode('utf-8')
status = libCtest.init(cp)

##################### PART END ###################

The second PART START / PART END will not cause the segfault in this context.

Would anyone know a reason to why?

VoltairePunk
  • 275
  • 4
  • 13
  • 2
    aside: `while(fgetc(pFile) != EOF) { ++fsize; }` is a very inefficient way to compute a file size. You could try `fseek` then `ftell` or `stat` – Jean-François Fabre Oct 26 '17 at 11:56
  • also could you show us the caller ? – Jean-François Fabre Oct 26 '17 at 11:57
  • Since it's a library and needs to be cross-platform compatible, won't this cause issues for (using ftell) on platforms like windows? I have read that this is most cross-platform methodology. Can you elaborate on "the caller"? You mean the parent function(s) where this is called from? – VoltairePunk Oct 26 '17 at 11:58
  • 2
    `ftell` is OK as long as you're opening the file in binary (else it's not equivalent to reading char by char). Yes, i'd like to see which function(s) call your funciton. Your function seems OK – Jean-François Fabre Oct 26 '17 at 12:01
  • 1
    Can't see much wrong here. The 'pFile = NULL;' at the end is pointless, but inoffensive. The two calls to 'fclose(pFile);' could probably be moved to just one, above the 'if', again just a niggle. I'm betting that UB somewhere else is stuffing you up:( – Martin James Oct 26 '17 at 12:10
  • @Jean-FrançoisFabre Such a method is not recommended because of UB. – BLUEPIXY Oct 26 '17 at 12:13
  • @BLUEPIXY which method? can you elaborate? (you know I take your comments _very_ seriously :)) – Jean-François Fabre Oct 26 '17 at 12:16
  • In `rewind` do you reset the stream position? – qwn Oct 26 '17 at 12:19
  • @qwn I believe so, yes. I have also amended my question to some additional context – VoltairePunk Oct 26 '17 at 12:24
  • @Jean-FrançoisFabre **7.21.9.2 The fseek function p3** _A binary stream need not meaningfully support fseek calls with a whence value of SEEK_END._ So In the case of binary stream, moving to `SEEK_END` does not necessarily represent the size of the file. – BLUEPIXY Oct 26 '17 at 12:26
  • @karmalis After you first call to the function all the subsequent calls return EOF which in int is -1 if you have not reset the stream, could we see the function for rewind? – qwn Oct 26 '17 at 12:27
  • @BLUEPIXY binary stream, like `popen` ok, but not in that case where this is a `FILE`. Note that reading all the data from a stream that you cannot rewind to get its size isn't an option either. – Jean-François Fabre Oct 26 '17 at 12:27
  • @qwn this particular rewind is part of `stdio.h` – VoltairePunk Oct 26 '17 at 12:28
  • @Jean-FrançoisFabre I recognize that the standard says not because it can not be rewound but because it allows padding. Anyway, be aware that moving to `SEEK_END` for a binary stream may have no meaning. – BLUEPIXY Oct 26 '17 at 12:30
  • @karmalis so `if (!cp.pkey_data) {`: first time it should read the file but the second time it should skip that part right? – Jean-François Fabre Oct 26 '17 at 12:32
  • @BLUEPIXY so you're recommending `stat` on the filename then? – Jean-François Fabre Oct 26 '17 at 12:32
  • @Jean-FrançoisFabre After all, if you are pursuing portability, you only have to break out such OS dependent libraries with something like `#IF..#ENDIF`, or read one character at a time as OP does. – BLUEPIXY Oct 26 '17 at 12:35
  • @Jean-FrançoisFabre I guess so, however I am testing all sorts of behavior, including the case where a new `_conn_param` isn't generated from Python side, which seems to cause this odd issue. My thought was: If you really decide to try the same thing with same parameters, the default behavior should be simply to re-do everything all over again. I am trying to avoid saving any data inside of C code and library itself. – VoltairePunk Oct 26 '17 at 12:35
  • 1
    also: who is responsible for freeing `buffer` ? – Jean-François Fabre Oct 26 '17 at 12:43
  • @Jean-FrançoisFabre good point. It's a structure that is created in Python and passed to C, so might be a good idea to implement some cleaning up after all is done. – VoltairePunk Oct 26 '17 at 12:50
  • In this case, the only reason to read the stream twice is to avoid having to dynamically resize the buffer. That strikes me as false economy; dynamic resizing is not expensive, and doing it that way allows it ro work with any stream, including those which cannot be rewound. – rici Oct 26 '17 at 13:38
  • and also: *buffer has a size of "size of the file plus 1". Is the caller determining the size of the (text) file as a C string and in that case, wouldn't it need to be nul-terminated? – Jean-François Fabre Dec 18 '17 at 20:22
  • In your question you ask us to ignore the stuff that begins with underscores, knowing someone will ask about it... why don't *you* strip them, since *you're* the one who wants an answer to this question, surely that'd be in *your* best interest? Replace `_ERROR` with `EXIT_FAILURE` or something... and while you're at it, write a `main` function which actually calls this code (the way you use it), and make sure the testcase compiles... – autistic Aug 21 '18 at 05:25
  • @autistic This is C a library. It does not need a `main` function as those are only for runnable applications. Honestly, noone has asked about the functions and underscores for almost a year, so I guess it's fine. Also, I have solved the issue. – VoltairePunk Aug 21 '18 at 08:19
  • The problem is that we can't reproduce the issue. Believe me, I've tried. I filled in the blanks and the program runs without segfault, so the problem must be in what you're not showing us... and producing an MCVE would've revealed that. – autistic Aug 22 '18 at 14:21
  • [See for yourself](http://codepad.org/6TXRhYG9)... Now if you would please follow the steps I suggested above to produce a minimal, compilable testcase that actually demonstrates the symptoms you describe, rather than some code which you think demonstrates the symptoms but you've not yet proven... that'd be best for you... – autistic Aug 22 '18 at 14:23
  • *"Functions in these libraries use the standard C calling convention, and are assumed to return `int`."* -- [`ctypes` manual on "Loading shared libraries"](https://docs.python.org/3/library/ctypes.html#loading-shared-libraries)... Are you sure `_status` isn't an alias for `int`? Why hide that from us? You should use `int` instead of `_status`... Disobeying the manual could cause segfaults, after all... – autistic Aug 22 '18 at 14:50
  • Also notice how [C11/7.1.3](https://port70.net/~nsz/c/c11/n1570.html#7.1.3) says *"All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use"* and *"All identifiers that begin with an underscore are always reserved for use as identifiers with file scope in both the ordinary and tag name spaces"*... and *"If the program declares or defines an identifier in a context in which it is reserved (other than as allowed by 7.1.4), or defines a reserved identifier as a macro name, the behavior is undefined."* – autistic Aug 22 '18 at 15:00

0 Answers0