Embarrassing error when reading string literal into a pointer

Question

I feel semi-retarded posting this, but I have no idea why my program is blowing up when it reads a string literal from the keyboard (i.e. then assigning it to a pointer).

Been debugging for over an hour and the program keeps blowing up when it reads from the keyboard.

I have tried everything to fix this. Initializing the string to a string literal (i.e. the compiler said it had a problem with nullptr). It is almost like I have an invisible character somewhere. If anyone can tell me what I'm doing wrong, I would greatly appreciate it.

main.cpp

#define _CRTDBG_MAP_ALLOC
#include <crtdbg.h>

#include <iostream>
#include <fstream>
#include "protocol.h"

int main()
{
_CrtSetDbgFlag(_CRTDBG_ALLOC_MEM_DF | _CRTDBG_LEAK_CHECK_DF);

int menuChoice = 0;

char * fileName = nullptr;
char * byteArray = nullptr;
char * hexArray = nullptr;
int numberOfBytes = 0;

PrintMenu();
GetMenuChoice(menuChoice);
ExecuteMenuChoice(menuChoice, fileName, byteArray, hexArray, numberOfBytes);

return 0;
}

protocol.cpp

void GetFile(char * fileName)
{

//Prompt user for binary file
std::cout << "\nEnter filename: " << std::endl;

//Read in location of binary file
std::cin.ignore(std::cin.rdbuf()->in_avail());
std::cin.getline(fileName, 256);
std::cin.clear();
std::cin.ignore(std::cin.rdbuf()->in_avail());
}

protocol.h

#ifndef PROTOCOL_H
#define PROTOCOL_H

//Function declarations
void PrintMenu();
void GetMenuChoice(int &menuChoice);
void ExecuteMenuChoice(int menuChoice, char *& fileName, char *& byteArray,
                   char *& hexArray, int numberOfBytes);
void NewLine();
void ThankUser();
void ErrorMessage();

#endif

Its not a string literal unless it is a string present in source code. — Karthik T, Mar 07 '13 at 02:11
You need to do `char fileName[255];` so it has a place to put the string. — Patashu, Mar 07 '13 at 02:12
I suspect `std::cin.getline(fileName, 256);` right away. But you don't even show how it is being used. Where is the minimal example reproducing the problem? — , Mar 07 '13 at 02:12
Retarded? I believe what you meant to say is you might have an [intellectual disability](http://blogs.suntimes.com/sweet/2010/10/obama_signs_rosas_law_mental_r.html)!! — ChiefTwoPencils, Mar 07 '13 at 02:15
Lose the `char *fileName` (for that matter *all* the `char *`s in this code) and use `std::string` instead. — WhozCraig, Mar 07 '13 at 02:17
@Patashu: Why `255`? Isn't that a little prone to buffer overruns? — johnsyweb, Mar 07 '13 at 06:22
@jJohnsyweb Not if you don't try to fill it with more than 255 characters :0 — Patashu, Mar 07 '13 at 08:16

score 2 · Answer 1 · edited Mar 07 '13 at 02:16

2

The code you have posted is incomplete but I don't see you allocating fileName before calling std::cin.getline(fileName, 256).

edited Mar 07 '13 at 02:16

Code-Apprentice

81,660
23
145
268

answered Mar 07 '13 at 02:13

Gary

5,642
1
21
41

score 1 · Answer 2 · answered Mar 07 '13 at 02:13

I don't see anywhere you call GetFile but it looks like it expects either an allocated buffer or an array of char. In main() you declare a char *fileName but don't allocate any memory for it. If you called (from main()) GetFile(fileName) then I would expect it to crash. You either need to allocate space in main() or in GetFile() for reading the data.

score 0 · Answer 3 · answered Mar 07 '13 at 02:23

Passing a string literal to be populated does not make any sense. You need to pass a buffer:

#include <iostream>
#include <fstream>
#include <stdlib.h> // for _MAX_PATH

int main()
{
    char filename[_MAX_PATH];

    GetFile(filename);

    std::cout << filename << std::endl;
}

_MAX_PATH makes a little more sense than hard-coding an arbitrary value like 256.

Better still would be use a std::string and remove those buffer sizes entirely!

#include <iostream>

void GetFile(std::string& fileName)
{
    std::cout << "\nEnter filename: " << std::endl;

    std::getline(std::cin, fileName);
}

int main()
{
    std::string filename;

    GetFile(filename);

    std::cout << filename << std::endl;
}

Thank you guys and sorry for the stupid question. Got it working. :) — MrPickle5, Mar 07 '13 at 03:09
@MrPickle5: Happy to help. [When you have decided which answer is the most helpful to you, mark it as the accepted answer by clicking on the check box outline to the left of the answer.](http://stackoverflow.com/faq/#howtoask) — johnsyweb, Mar 07 '13 at 04:11

Peixu Zhu · Answer 4 · 2013-03-08T09:14:59.067

0

cin::getline() does not allocate memory for you to store the string literals into fileName. You must provide memory space for it, by allocating memory to fileName, just like

fileName = new char[_MAX_PATH];

at last, don't forget to release the memory by

delete[] fileName;

edited Mar 08 '13 at 09:14

answered Mar 07 '13 at 02:26

Peixu Zhu

2,111
1
15
13

`malloc()` and `free()`? Isn't this question tagged [tag:c++]? – johnsyweb Mar 07 '13 at 02:28
c++ does not reject `malloc` & `free`, it is multi-paradigm. However, `new` & `free` should be a better choice :-) – Peixu Zhu Mar 08 '13 at 07:09
Not rejecting these functions does not mean that they are idiomatic C++. Using `new` as you have edited is closer to C++ style, but this needs to be coupled with `delete []`. Since you know the size at compile time, it would make more sense to allocate the memory on the stack as in my answer. – johnsyweb Mar 08 '13 at 08:00
thanks :-) I'd prefer to use `std::string` in practice, rather than allocate memory on stack, though the size is known at compile time. I like to keep stack small, to avoid potential stack overflow as possible. – Peixu Zhu Mar 08 '13 at 09:22

Embarrassing error when reading string literal into a pointer

4 Answers4