'unsigned long int', and 'unsigned long long int' assignment issue

Question

A few weeks ago, I was using for first time (I'm not used to use them) floats, doubles, and I have some problems with the comparison operand. I also have had problems while trying to assign values to that types, but I've solved it as well...

Today, I'm making a library in C++, and I have found an error... well... strange? or just my stupid thinking?

This is the code:

ini::ini(const char * path, bool _autoflush_ /*= false*/) {
/* Storing file name ... */
f_name = new char[strlen(path)+1];
strcpy(f_name, path);

/* Storing autoflush ... */
autoflush = _autoflush_;

/* First step: getting file size */
    /* Open the file in read/append mode */
    FILE * fd = fopen(path, "r");
    /* On non-valid descriptor, goto next step directly */
    if(fd == NULL) f_size = 1; goto allocbuffer;

    /* Seek to the end */
    fseek(fd, 0, SEEK_END);
    /* Get file size */
    f_size = (unsigned long int)ftell(fd) + 1;

/* Second step: allocating memory for the buffer */ allocbuffer:
    cout << endl << endl << endl << endl << "Wanting " << sizeof(char)*f_size << " bytes of memory!" << endl << endl << endl << endl;
    /* Allocate buffer-space */
    buffer = (char*)malloc(sizeof(char)*f_size);
    if(buffer == NULL) {
        errord = (char*)malloc(strlen(INI_ERROR_NOT_ENOUGH_MEMORY) + 1);
        strcpy(errord, INI_ERROR_NOT_ENOUGH_MEMORY);
        cout << endl << endl << endl << endl << "Last error: \"" << errord << "\"." << endl << endl << endl << endl;
        return;
    }
    /* Initialize and fill it with null bytes */
    memset(buffer, 0, f_size);
    /* Goto next step */
    if(fd == NULL) goto endconstruct;

/* Third step: storing in the buffer */ loadbuffer:
    /* Rewind file descriptor */
    rewind(fd);
    /* Read from file */
    if(fread(buffer, 1, f_size, fd) != f_size) {
        errord = (char*)malloc(strlen(INI_ERROR_NOT_READED) + 1);
        strcpy(errord, INI_ERROR_NOT_READED);
        cout << endl << endl << endl << endl << "Last error: \"" << errord << "\"." << endl << endl << endl << endl;
        cout << endl << endl << endl << endl << "BYTES OF FILE: \"" << f_size << "\"." << endl << endl << endl << endl;
    }
    /* Close file descriptor */
    fclose(fd);
    /* Get number of lines */
    f_line = strnum(buffer, "\n") + 1;

/* End building of object */
endconstruct:
    /* Print out what is stored in the buffer NOW */
    cout << endl << endl << endl << endl << "Buffer is:" << endl << buffer << endl << endl << endl << endl;

return;

}

Probably ini libraries are already created, and much better than my. But I'm starting learning C++ from C, and I want to practice with something interesting and useful. I missed the class declaration, I don't know if it's necessary to paste it here, but here is:

    /** @def INI_ERROR_NOT_READED
    @brief <em>Not readed as many bytes as required</em>
*/
#define INI_ERROR_NOT_READED "Not readed as many bytes as required"
/** @def INI_ERROR_NOT_ENOUGH_MEMORY
    @brief <em>There is not enough memory</em>
*/
#define INI_ERROR_NOT_ENOUGH_MEMORY "There is not enough memory"

/** @class ini
    @brief Class to describe <em>ini</em> files.

    It describes an ini file. All the file is readed and loaded
    in memory, for faster access. This class is the
    improved & C++ version of the old, monstruous
    functions defined in the old, monstruous IO Utilities
    Library. Writting functions use dynamic memory reallocation
    and file flush to the filesystem.
*/
class ini {
    public:
        /** @brief Constructor. Gives initial memory for the buffer and loads all the file in that buffer.
         * 
         * @param path - Path of the <em>ini</em> file to open.
         * @param _autoflush_ - Whether to auto-flush changes to hard disk or not.
         *  If you don't set it to any value, <em>false</em> is taked as default
         *  value and you have to flush changes manually using member function flush().
         *  Setting it to <em>true</em> may make it less efficient, so be careful
         *  if you're going to make a lot of changes in the <em>ini</em> file.
        */
        ini                     (const char * path, bool _autoflush_ = false);
        /** @brief Destructor. Frees the memory pointed by <em>buffer</em> and destroys the #ini object.
         * 
         * It's very important to free the memory buffer, to avoid memory corruptions.
        */
        ~ini                    (void);
        /** @brief Gets last error stored in private member <em>errord</em>.
         * 
         * @return Last error-descriptor as string.
        */
        char *      geterror    (void);
        /** @brief Flush changes made in the buffer to the hard disk.
         * 
         * You can do it manually or set auto-flushing by the second argument of
         * ini::ini().
         * 
         * @par Example of usage:
         * @code
         *  ini inid("myini.ini");
         *  // make changes
         *  inid.flush();
         * @endcode
        */
        void        flush       (void);
        /** @brief Flush changes made in the buffer to *another* file the hard disk.
         * 
         * Using this function instead of normal flush(void), you are able to
         * save the buffer to another #ini file that is not the original one.
         * 
         * @par Example of usage:
         * @code
         *  ini inid("myini.ini");
             *  // make changes
         *  inid.flush("myini.backup.ini");
         * @endcode
        */
        void        flush       (const char * path);
        /** @brief Checks if a section exists.
         * 
         * @param tsection - The name of the section to check, without the braces.
         * 
         * @return #true if the section exists; #false if not.
        */
        bool        sectExists  (const char * tsection);
        /** @brief Gets the line in that a section starts.
         * 
         * @param tsection - The name of the section to check, without the braces.
         * 
         * @return The line in that the section starts; -1 if not-founded section.
         *  Keep in mind that the first line is 1, the second, 2,...
        */
        int         sectStart   (const char * tsection);
        /** @brief Gets the line in that a section ends.
         * 
         * @param tsection - The name of the section to check, without the braces.
         * 
         * @return The line in that the section ends; -1 if not-founded section.
         *  Keep in mind that the first line is 1, the second, 2,...
        */
        int         sectStop    (const char * tsection);
        /** @brief Checks if a key exists.
         * 
         * @param tsection - The name of the section to check, without the braces.
         *  If the key is outside any section (if it's a #KWOS), then <em>tsection</em>
         *  should be #KWOS.
         * @param tkey - The name of the key to check.
         * 
         * @return #true if the key exists in the specified section; #false if not.
        */
        int         keyExists   (const char * tsection, const char * tkey);
        /** @brief Reads the value of a key as a string.
         * 
         * @param tsection - The name of the section to read from, without the braces.
         *  If the key is outside any section (if it's a #KWOS), then <em>tsection</em>
         *  should be #KWOS.
         * @param tkey - The name of the key to read its value.
         * @param tval - The default string to return if cannot found the key.
         * 
         * @return The value of the key <em>tkey</em> in section <em>tsection</em>; or
         *  <em>tval</em> when non-existing key.
        */
        char *      read        (const char * tsection, const char * tkey, const char * tval);
        /** @brief Reads the value of a key as an integer value.
         * 
         * @param tsection - The name of the section to read from, without the braces.
         *  If the key is outside any section (if it's a #KWOS), then <em>tsection</em>
         *  should be #KWOS.
         * @param tkey - The name of the key to read its value.
         * @param tval - The default value to return if cannot found the key.
         * 
         * @return The value of the key <em>tkey</em> in section <em>tsection</em>; or
         *  <em>tval</em> when non-existing key.
        */
        long int    readi       (const char * tsection, const char * tkey, int tval);
        bool        delKey      (const char * tsection, const char * tkey);
        bool        delSect     (const char * tsection);
        bool        write       (const char * tsection, const char * tkey, const char * tval);
        bool        write       (const char * tsection, const char * tkey, int tval);
    private:
        unsigned long int       f_size; /**< File size. */
        unsigned int            f_line; /**< Number of lines of the <em>ini</em> file. */
        char *                  buffer; /**< Memory buffer to store data. Dynamimcally reallocated. */
        char *                  f_name; /**< File name. */
        bool                 autoflush; /**< Whether to auto-flush to hard disk or not. */
        char *                  errord; /**< Last error stored internally by the functions of the #ini class. */
};

After a few 'tests', I have finally discovered that the problem is in the 'f_size' variable. Why? Don't know. But, if I print it out to stdout, it displays a very very big number. That's the problem of the memory-assignment (with malloc) error and the subsequent error at reading from file (or initialization with memset).

Help is much appreciated. And links, refs or explanations to have me see my error, and continue learning.

Thanks!

P.S.: I'm using g++ in a Linux Debian "squeeze", amd64.

Answer 1

Here is one subtle but significant issue:

 if(fd == NULL) f_size = 1; goto allocbuffer;

If the file does exist, the logic still jumps to label allocbuffer.

To fix this, use braces. And indent for crystal clarity.

 if(fd == NULL)
 {
      f_size = 1;
      goto allocbuffer;
 }

By skipping to the exit, variables are not initialized properly.

Answer 2

In addition to the issue pointed out by wallyk, whenftell encounters an error it returns -1 and casting that to an unsigned long is causing an overflow.

In any case, don't use ftell to get a file's size. Use fstat or similar.

Answer 3

I won't debug the code for you (this more belongs to codereview.se), but here are some hints:

f_name = new char[strlen(path)+1];
strcpy(f_name, path);

Instead: define f_name as std::string and do a...

f_name = path;

---

        errord = (char*)malloc(strlen(INI_ERROR_NOT_READED) + 1);
        strcpy(errord, INI_ERROR_NOT_READED);
        cout << "Last error: \"" << errord << "\"." << endl;

Are you, like, dynamically allocating memory just to write a string into the screen? Instead do simply:

        errord = INI_ERROR_NOT_READED;
        cout << "Last error: \"" << errord << "\"." << endl;

---

Also to read a whole file into a buffer, there are simpler ways (as in, a few lines of code). See https://stackoverflow.com/a/2602060/399317 .

---

Long story short: new[] and malloc aren't your friends unless you're writing your own containers. Use STL containers instead (std::vector for instance, std::string is helpful too).