1

I'm writing a function (*rwObjects()) that will read a formatted file and save it's strings, one object at a time. Unfortunately, it's for my studies which have a restriction - stdio.h, stdlib.h and string.h is practically all I can use.

Here's the problem: whenever I run the code, when it gets to the fclose(input), VS17 says my project's triggered a breakpoint, and then opens a tab that says "wntdll.pdb not loaded" or something.

The question is: how do I not trigger the breakpoint and close the file properly? Or, if the problem isn't in the file, where is it?

Code (C):

#define _CRT_SECURE_NO_WARNINGS
#define cnCOUNTRY_LENGTH 3
#define cnOBJECT_NAME_LENGTH 30
#define cnOBJECT_MAX 1000

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

//--Поле объекта objectType--
//Country (0)       - строка названия страны
//ObjectName (1)    - строка названия объекта
//Square (2)        - площадь объекта
//Error (3)         - ошибка в содержании строки
typedef enum IOOptions {Country, ObjectName, Square, Error} IOType;

//--Тип обрабатываемых объектов--
//char Country      - строка названия страны
//char ObjectName   - строка названия объекта
//int Square        - площадь объекта
typedef struct object {
    char Country[cnCOUNTRY_LENGTH];
    char ObjectName[cnOBJECT_NAME_LENGTH];
    int Square;
} objectType;

//--Копирование текущего элемента строки objects.txt--
//strMod            - Строка, в которую идёт копирование
//strPos            - Позиция в считываемой строке
//strBlueprint      - Строка, из которой идёт копирование
//writeType         - Поле объекта objectType. При "Country" - переводит вводимые символы в верхний регистр ('a' -> 'A' и т.д.)
void copyInputStr(char *strMod, int *strPos, char *strBlueprint, IOType writeType) {
    for (*strPos; *strBlueprint != ' ' && *strBlueprint != '\n' && *strBlueprint != NULL; *strPos = *strPos + 1) {
        *strMod = *strBlueprint;
        if (writeType == Country) toupper(*strMod);
        strBlueprint++; strMod++;
    }
}

//--Запись текущего элемента строки objects.txt в текущий объект--
//strInput          - Строка, из которой идёт запись
//objectOutput      - Объект, в который идёт запись
//writeType         - Поле объекта, в которое идёт запись
void writeObject(char *strInput, objectType *objectOutput, IOType writeType) {
    if (writeType == Country)
        strcpy(objectOutput->Country, strInput);
    else if (writeType == ObjectName)
        strcpy(objectOutput->ObjectName, strInput);
    else if (writeType == Square)
        objectOutput->Square = atoi(strInput);
    else printf("Error 1. Invalid parameters");
}

//--Чтение objects.txt и запись в массив objectType--
//Возвращает указатель на первый элемент массива объектов
objectType *rwObjects() {
    FILE *input = fopen("objects.txt", "r");
    char objectQttStr[4], objectStr[38];
    fgets(objectQttStr, 4, input);
    objectType *objectList = (objectType *)malloc(atoi(objectQttStr)), *currentObject = objectList;
    currentObject = (objectType *)malloc(atoi(objectQttStr));
    for (int i = 0; i < atoi(objectQttStr); i++) {
        fgets(objectStr, 38, input);
        IOType inputType = Country;
        for (int j = 0; objectStr[j] != NULL && objectStr[j] != '\n'; j++) {
            char strBuf[cnOBJECT_NAME_LENGTH];
            memset(&strBuf, 0, sizeof(strBuf));

            copyInputStr(&strBuf, &j, &objectStr[j], inputType);

            writeObject(&strBuf, currentObject, inputType);

            inputType++; 
        }
        currentObject++;
    }
    fclose(input);         //this is where it happens
    return objectList;
}

void main() {
    objectType *objectList = rwObjects();
    printf("");
}

This is one confusing program, but I've found no other way to conform the bloody rules, so let's put the coding style aside, shall we?

Also, I know that should it run successfully, nothing would happen - that is by design. It's not finished yet.

EDIT: Don't worry about validity of input data. All the input data formatting is explicitly stated in the task, ergo no checks are reqired. Still, for the curious, here it is:

objects.txt:

3
USA WelfareArrangement 120
Rus PoiskZemli 30
usa asdfEstate 1

EDIT 2: The moment I stopped using malloc, everything was fine. The question is - why exactly was it such a problem, and how would I create an array of the exact size I need, instead of creating max size evey time, if not with malloc?

Johnny Cache
  • 1,033
  • 7
  • 12
  • I imagine you have a breakpoint set somewhere, perhaps inside the standard library code. Is there a way in VS17 to view/list all breakpoints? – Oliver Charlesworth Dec 19 '17 at 18:23
  • `objectStr[j] != NULL` looks like code is comparing a `char` to a pointer. Recommend `objectStr[j] != '\0'` instead. – chux - Reinstate Monica Dec 19 '17 at 18:28
  • @OliverCharlesworth Tried. Only lists the breakpoints in my project, not in the outside files. If there are any, I wasn't the one who put them there. And no, it doesn't work if I get rid of all my breakpoints. – Johnny Cache Dec 19 '17 at 18:28
  • 1
    I wonder if there's a write out of bounds corrupting `input`, maybe in `copyInputStr`? – David Schwartz Dec 19 '17 at 18:30
  • 2
    Your memory allocations make no sense. You seem to be allocating an array of structures, but you only give them one byte each. Then you assign a current pointer, which you then overwrite with a new useless allocation. – Lee Daniel Crocker Dec 19 '17 at 18:30
  • 1
    Code should check the return value of `fgets(objectStr, 38, input);`. – chux - Reinstate Monica Dec 19 '17 at 18:31
  • 1
    Whatever warning level you're using, it isn't high enough. [A sample of warnings from clang here](https://pastebin.com/CVQYXAqe). – WhozCraig Dec 19 '17 at 18:32
  • Post your sample input. `fgets(objectQttStr, 4, input);` looks quite small for an integer. – chux - Reinstate Monica Dec 19 '17 at 18:32
  • 1
    I think you're hitting an assert or a VS error, not a breakpoint. I don't use VS, so I can't look further, but I suspect that you've corrupted your input pointer somehow and fclose is complaining. – Erik Johnson Dec 19 '17 at 18:33
  • @LeeDanielCrocker How do I assign them one byte? I thought I assign them just enough to contain objectQttStr number of objectType structures. And VS seems to agree, judging by the control values window. – Johnny Cache Dec 19 '17 at 18:42
  • I'm assuming objQttStr means something like "object quantity"? It's passed to malloc directly. malloc's argument is a number of bytes, not a number of objects. Perhaps you meant to use calloc? – Lee Daniel Crocker Dec 19 '17 at 18:45
  • @LeeDanielCrocker objQttStr is passed wrapped in atoi(). So the value is passed. Honestly, I'm not quite sure how malloc works, but the way I see it, it allocates (type *) malloc (amount), which means it allocates exactly type times amount memory. At least it's true for any other type. Maybe using a structure is the problem. – Johnny Cache Dec 19 '17 at 18:51
  • Nope. Malloc allocates bytes, period. You need to do that multiplication yourself, or else you'll be shitting all over unallocated memory and causing faults...hey... – Lee Daniel Crocker Dec 19 '17 at 19:01
  • @LeeDanielCrocker Interesting. So the things taught to us in our university are lies. I'll try to avoid allocating weird things and see if it works. Also, c can burn in hell. – Johnny Cache Dec 19 '17 at 19:05

1 Answers1

2

First problem:

objectType *objectList = (objectType *)malloc(atoi(objectQttStr)), *currentObject = objectList;
currentObject = (objectType *)malloc(atoi(objectQttStr));

The malloc function allocates a given number of bytes. So if you have 5 objects, you only allocate 5 bytes. That's not enough for your structs. This results in you writing past the end of allocated memory which invokes undefined behavior.

If you want it to allocate space for a specific number of objects, you need to multiply by the object size:

objectType *objectList = malloc(sizeof(*objectList)*atoi(objectQttStr));

Also, don't cast the return value of malloc.

You also assign currentObject to the same value as objectList but then overwrite it with a separate memory allocation. So get rid of the second malloc.

Second problem:

        memset(&strBuf, 0, sizeof(strBuf));

        copyInputStr(&strBuf, &j, &objectStr[j], inputType);

        writeObject(&strBuf, currentObject, inputType);

Your copyInputStr and writeObject function expect a char *, but you pass in the address of the strBuf array which has type char (*)[30]. Get rid of the address-of operator here:

        memset(strBuf, 0, sizeof(strBuf));

        copyInputStr(strBuf, &j, &objectStr[j], inputType);

        writeObject(strBuf, currentObject, inputType);

Third problem:

void copyInputStr(char *strMod, int *strPos, char *strBlueprint, IOType writeType) {
    for (*strPos; *strBlueprint != ' ' && *strBlueprint != '\n' && *strBlueprint != NULL; *strPos = *strPos + 1) {
        *strMod = *strBlueprint;
        if (writeType == Country) toupper(*strMod);
        strBlueprint++; strMod++;
    }
}

When you copy the characters in strMod, you're not adding a null byte at the end. A string in C is a null-terminated array of characters, so what you end up with is not a string but just an array of characters. When you later call strcpy on this array, the function doesn't find a null byte so it keeps reading until it does. This causes the function to read uninitialized bytes and/or read past the end of the array, which again invokes undefined behavior.

Add the null terminating byte after the loop. Also, the result of the toupper function isn't assigned to anything, so it does nothing. You need to assign it back to *strMod:

void copyInputStr(char *strMod, int *strPos, char *strBlueprint, IOType writeType) {
    for (*strPos; *strBlueprint != ' ' && *strBlueprint != '\n' && *strBlueprint != NULL; *strPos = *strPos + 1) {
        *strMod = *strBlueprint;
        if (writeType == Country) *strMod = toupper(*strMod);
        strBlueprint++; strMod++;
    }
    *strMod = 0;
}
dbush
  • 205,898
  • 23
  • 218
  • 273
  • You, sir, are amazing. You didn't only answer my question in the most possible depth, you also solved a few problems I've been at myself (even though I've figured them out meanwhile). You deserve infinite praise. – Johnny Cache Dec 19 '17 at 19:32