Sign in to follow this  
dphoenix

Having a problem with a vector iterator

Recommended Posts

Good afternoon, I'm at my wits end. I'm keeping track of a vector of 2 IP addresses. I do a function to send them all a message via TCP. Then when I go back and check right after I send the message, the first element of the vector is changed to the second, and they are both the same! This is where I iterate and send message to all the addresses:
vector<const char*>::battleList;
vector<const char*>::iterator it;

        for(it = battleList.begin(); it !=battleList.end(); it++)
        {
            Message m;
            cout<<"IP: "<<*it<<endl;
            messenger.sendMessage(m, *it);
        }
Here's the messenger function:
void Messenger::sendMessage(Message m, const char* host)
{
    int n, sockfd;
    struct sockaddr_in serv_addr;
    struct hostent *server = gethostbyname(host);

    string m1 = string(m.code) + "#";
    string m2 = m.message;
    string m3 = m1 + m2;

    const char* buffer1 = m3.c_str();

    char buffer[256];
    memcpy(buffer, buffer1, sizeof(buffer1));

    sockfd = socket(AF_INET, SOCK_STREAM, 0);
    if (sockfd < 0)
    {
        cout<<"ERROR opening socket"<<endl;
        int t;
        cin>>t;
        if (server == NULL) {
        fprintf(stderr,"ERROR, no such host\n");
        exit(0);
        }
    }

    bzero((char *) &serv_addr, sizeof(serv_addr));
    serv_addr.sin_family = AF_INET;
    bcopy((char *)server->h_addr,
         (char *)&serv_addr.sin_addr.s_addr,
         server->h_length);
    serv_addr.sin_port = htons(9000);

    if (connect(sockfd,(struct sockaddr*)&serv_addr,sizeof(serv_addr)) < 0)
        cout<<"ERROR connecting"<<endl;

    n = write(sockfd,buffer,strlen(buffer));
    n = send(sockfd, buffer, strlen(buffer), 0);
    if (n < 0)
         cout<<"ERROR writing to socket"<<endl;

}
As you can see, nowhere in the messaging code did I change the host address. In fact, if I put in a cout statement in the for loop, right after the message is sent, it says that the address has not been changed. But if I do:
cout<<battleList[0]<<endl;
cout<<battleList[1]<<endl;
immediately after the for loop, the first address has changed. I called that same bit of code right before the for loop to see if there was a difference. What on earth can the problem be? No one else is accessing these pointer values. If I take the sendMessage function out of the for loop, and just output the address, there is no problem. But I don't see the problem in sendMessage, and in fact, if I cout the host address right at the end of that function, it still is correct. The problem seems to occur in the handoff back to my for loop.

Share this post


Link to post
Share on other sites
I'm not entirely sure, but why on earth are you keeping a vector of char*'s? Where is that allocated? If it's from a std::string::c_str() call, or a string on the stack, you're likely not copying the string out, just storing thr pointer. I suspect you really want to store a vector of std::strings, unless you're storing pointers to arbitrary chunks of memory.

Share this post


Link to post
Share on other sites
Quote:
Original post by Evil Steve
I'm not entirely sure, but why on earth are you keeping a vector of char*'s? Where is that allocated? If it's from a std::string::c_str() call, or a string on the stack, you're likely not copying the string out, just storing thr pointer. I suspect you really want to store a vector of std::strings, unless you're storing pointers to arbitrary chunks of memory.



The vector of char*s is because the TCP protocol stuff takes addresses in that format, rather than a string. It's just easier than having to convert back and forth.

Share this post


Link to post
Share on other sites
Quote:
Original post by dphoenix
Quote:
Original post by Evil Steve
I'm not entirely sure, but why on earth are you keeping a vector of char*'s? Where is that allocated? If it's from a std::string::c_str() call, or a string on the stack, you're likely not copying the string out, just storing thr pointer. I suspect you really want to store a vector of std::strings, unless you're storing pointers to arbitrary chunks of memory.



The vector of char*s is because the TCP protocol stuff takes addresses in that format, rather than a string. It's just easier than having to convert back and forth.
So how are you filling the vector? If you're doing something like this:

char szBuff[256];
sprintf(szBuff, "123.123.123.123");
battleList.push_back(szBuff);
sprintf(szBuff, "0.0.0.0");
battleList.push_back(szBuff);


Then battleList will just store the szBuff pointer both times, which is a random varaiable on the stack. Only the pointer is copied, not the contents. If you were storing a vector of std::strings, then push_back() would implicitly convert the char* to a std::string, and would copy the data (not the pointer).

If the value changes depending on calling a function, that usually means that the function is changing the stack (Which isn't a problem), and that's affecting what your code does.

Basically - How are you adding elements to the vector [smile]?

Share this post


Link to post
Share on other sites
Hi again :)

I am doing push_back. So when I enter combat with an enemy, I take my IP address and theirs and add it like so:


const char* myIP; //initialized to whatever
const char* theirIP;

battleList.push_back(myIP);
battleList.push_back(theirIP);


If I do a check to confirm the IP addresses immediately after this part, they display as they should.

Share this post


Link to post
Share on other sites
Quote:
Original post by dphoenix

const char* myIP; //initialized to whatever
const char* theirIP;
Define "whatever". You need to allocate space for the pointer to point at at some point, and if both pointers end up pointing at the same space or similar you could quite easily get the problem you're describing.

Share this post


Link to post
Share on other sites
I think it's a pointer issue.

struct hostent *server = gethostbyname(host);

I'm not sure what "gethostbyname" does, but are you setting "server" to the same address location as "host"

bcopy will overlay memory if the two areas overlap.

From http://www.opengroup.org/onlinepubs/000095399/functions/bcopy.html
The bcopy() function shall copy n bytes from the area pointed to by s1 to the area pointed to by s2.

The bytes are copied correctly even if the area pointed to by s1 overlaps the area pointed to by s2.

Share this post


Link to post
Share on other sites
Quote:
Original post by Cranny76
I think it's a pointer issue.

struct hostent *server = gethostbyname(host);

I'm not sure what "gethostbyname" does, but are you setting "server" to the same address location as "host"

bcopy will overlay memory if the two areas overlap.

From http://www.opengroup.org/onlinepubs/000095399/functions/bcopy.html
The bcopy() function shall copy n bytes from the area pointed to by s1 to the area pointed to by s2.

The bytes are copied correctly even if the area pointed to by s1 overlaps the area pointed to by s2.
gethostbyname() stores a static hostent struct internally, and returns that. I presume at least, it allocates the storage space anyway and doesn't require you to free it.

Another thing the OP could try is setting a data breakpoint (After checking that there's different data being stored and not two copies of the same pointer or something). That'll break to the debugger when the specified memory changes.

Share this post


Link to post
Share on other sites
This is the function to get myIP:



const char* Messenger::getMyIP()
{
hostent* localInfo = gethostbyname(""); // get the local host name
struct in_addr addr;
memcpy(&addr,localInfo->h_addr_list[0],sizeof(struct in_addr));
const char* localIP = inet_ntoa(addr);
return localIP;
}




I've confirmed that that part works.

Also, whenever another player sends me a message, the message automatically has their IP address attached by the TCP listener that gets the message:




string mymessage(buffer);
size_t found = mymessage.find("#");
if((found != string::npos) && (found > 0))
//found # delimiter
{
Message m;
int pos = int(found);
string codeSub = mymessage.substr(0, pos);
m.code = codeSub;
m.message = mymessage.substr(pos+1);
m.sender = host;
messageList.push_back(m);
//cout<<"Message in queue."<<endl;
}


The preceding code is in the TCP listener, when it gets a message from another player. Each message has a code and the content, all delineated by the # sign. So that's what m.code and m.message are. Each message also has a space for the sender address, which is initially uninitialized when its sent (this was before I knew how to get my own IP address). m.sender is also a const char*,


class Message
{
public:
Message();
Message(string c, string m);

public:
string code;
string message;
const char* sender;
};

Message::Message()
{}

Message::Message(string c, string m):code(c),message(m)
{}





That's what it looks like. I hope I didn't post too much irrelevant code, but I want to make sure I'm providing enough info because I am absolutely at my wit's end and ready to cry right now lol...

Share this post


Link to post
Share on other sites
Quote:
Original post by Evil Steve
Quote:
Original post by Cranny76
I think it's a pointer issue.

struct hostent *server = gethostbyname(host);

I'm not sure what "gethostbyname" does, but are you setting "server" to the same address location as "host"

bcopy will overlay memory if the two areas overlap.

From http://www.opengroup.org/onlinepubs/000095399/functions/bcopy.html
The bcopy() function shall copy n bytes from the area pointed to by s1 to the area pointed to by s2.

The bytes are copied correctly even if the area pointed to by s1 overlaps the area pointed to by s2.
gethostbyname() stores a static hostent struct internally, and returns that. I presume at least, it allocates the storage space anyway and doesn't require you to free it.

Another thing the OP could try is setting a data breakpoint (After checking that there's different data being stored and not two copies of the same pointer or something). That'll break to the debugger when the specified memory changes.


How do I set a data breakpoint? I'm using Code::Blocks because this is crossplatform and needs cygwin. Otherwise I'd be using the Visual Studio debugger to trace the data changes.

Share this post


Link to post
Share on other sites
Quote:
Original post by dphoenix
This is the function to get myIP:



const char* Messenger::getMyIP()
{
hostent* localInfo = gethostbyname(""); // get the local host name
struct in_addr addr;
memcpy(&addr,localInfo->h_addr_list[0],sizeof(struct in_addr));
const char* localIP = inet_ntoa(addr);
return localIP;
}




I've confirmed that that part works.
What if you call that function 2 or 3 times? I bet it returns the same pointer each time, meaning if you store the pointers in a vector, you'll end up with all entries in the vector being pointers to the same character, meaning they'll seem to be the same string.

Quote:
Original post by dphoenix
Also, whenever another player sends me a message, the message automatically has their IP address attached by the TCP listener that gets the message:




string mymessage(buffer);
size_t found = mymessage.find("#");
if((found != string::npos) && (found > 0))
//found # delimiter
{
Message m;
int pos = int(found);
string codeSub = mymessage.substr(0, pos);
m.code = codeSub;
m.message = mymessage.substr(pos+1);
m.sender = host;
messageList.push_back(m);
//cout<<"Message in queue."<<endl;
}


The preceding code is in the TCP listener, when it gets a message from another player. Each message has a code and the content, all delineated by the # sign. So that's what m.code and m.message are. Each message also has a space for the sender address, which is initially uninitialized when its sent (this was before I knew how to get my own IP address). m.sender is also a const char*,
If m.sender is a const char*, it's just storing the pointer, not the contents. What is host?
Also, the test for fount > 0 and the cast of found to an it is redundant (std::string::find will always return npos or >= 0, and substr() takes size_t parameters rather than ints I think (Don't have VC to check though)).

Quote:
Original post by dphoenix

class Message
{
public:
Message();
Message(string c, string m);

public:
string code;
string message;
const char* sender;
};

Message::Message()
{}

Message::Message(string c, string m):code(c),message(m)
{}





That's what it looks like. I hope I didn't post too much irrelevant code, but I want to make sure I'm providing enough info because I am absolutely at my wit's end and ready to cry right now lol...
[smile]

Basically, whenever you store a const char* you're storing the pointer, not the contents. If you're re-using a buffer (Like the sprintf() example in my last post), then you're just storing the address of the original buffer, not a copy of it.

Share this post


Link to post
Share on other sites
I just tried something.

I tried both of the following statements:



cout<<(battleList.front() == myIP)<<endl;

cout<<(battleList.back() == myIP)<<endl;



Front returned false, back returned true. So it is the data being changed and not the pointers themselves...

Share this post


Link to post
Share on other sites
Quote:
Original post by dphoenix
This is the function to get myIP:



const char* Messenger::getMyIP()
{
hostent* localInfo = gethostbyname(""); // get the local host name
struct in_addr addr;
memcpy(&addr,localInfo->h_addr_list[0],sizeof(struct in_addr));
const char* localIP = inet_ntoa(addr);
return localIP;
}




I've confirmed that that part works.


Here's the thing: it doesn't.

First of all, the function is deprecated, as noted there. But more importantly,

Quote:
inet_ntoa() returns the dots-and-numbers string in a static buffer that is overwritten with each call to the function.


(boldface original, italic emphasis mine.)

Messenger::getMyIP() actually always returns the same value: the address of the static buffer of the inet_ntoa() function. What inet_ntoa() does is declare a 'static char[SOME_SIZE_VALUE]', put the dots-and-numbers string in there, and return a pointer to its static buffer. (This is only even safe, BTW, because of the static-ness; you can't properly return a pointer to a stack-allocated variable.) You need to make your own copy of the string data, or else it just gets overwritten with the next call.

This is why we use std::string, and wrap ugly C interfaces as tightly as possible. :)

Share this post


Link to post
Share on other sites
Ok, a data breakpoint should help track it down. If you're using VC2005 (2008 and 2003 should be similar), you can set a breakpoint in your code after the vector is intially set up, and then when the breakpoint is hit, go to Debug -> New Breakpoint -> New Data Breakpoint, and enter the address of the first element in the vector (Which you can see in the debugger).
Alternatively, step through the code and see when the line changes. If you're not familiar with Visual Studios debugger, see Superpig's excelent article.

Share this post


Link to post
Share on other sites
Quote:
Original post by Zahlman
Quote:
Original post by dphoenix
This is the function to get myIP:



const char* Messenger::getMyIP()
{
hostent* localInfo = gethostbyname(""); // get the local host name
struct in_addr addr;
memcpy(&addr,localInfo->h_addr_list[0],sizeof(struct in_addr));
const char* localIP = inet_ntoa(addr);
return localIP;
}




I've confirmed that that part works.


Here's the thing: it doesn't.

First of all, the function is deprecated, as noted there. But more importantly,

Quote:
inet_ntoa() returns the dots-and-numbers string in a static buffer that is overwritten with each call to the function.


(boldface original, italic emphasis mine.)

Messenger::getMyIP() actually always returns the same value: the address of the static buffer of the inet_ntoa() function. What inet_ntoa() does is declare a 'static char[SOME_SIZE_VALUE]', put the dots-and-numbers string in there, and return a pointer to its static buffer. (This is only even safe, BTW, because of the static-ness; you can't properly return a pointer to a stack-allocated variable.) You need to make your own copy of the string data, or else it just gets overwritten with the next call.

This is why we use std::string, and wrap ugly C interfaces as tightly as possible. :)


Wow...okay. So let me make sure I understand what happened. The address in the battleList that is being replaced is the other guy's address with mine, so I'm always sending myself messages but not the other guy. So basically, using the inet_ntoa() function overwrote his address with mine?

I guess I'd better go back and change the battleList to strings!

I'll make that change and then check back in with my results, if you guys don't mind.

Thanks!

Share this post


Link to post
Share on other sites
Quote:
Original post by Evil Steve
Ok, a data breakpoint should help track it down. If you're using VC2005 (2008 and 2003 should be similar), you can set a breakpoint in your code after the vector is intially set up, and then when the breakpoint is hit, go to Debug -> New Breakpoint -> New Data Breakpoint, and enter the address of the first element in the vector (Which you can see in the debugger).
Alternatively, step through the code and see when the line changes. If you're not familiar with Visual Studios debugger, see Superpig's excelent article.


Darn, see this is a time when it would be nice to be able to use Visual Studio. By far the best debugger I've used. I don't think Code::Blocks has that functionality (though I may be wrong!)

Share this post


Link to post
Share on other sites
Quote:
Original post by dphoenix
Wow...okay. So let me make sure I understand what happened. The address in the battleList that is being replaced is the other guy's address with mine, so I'm always sending myself messages but not the other guy. So basically, using the inet_ntoa() function overwrote his address with mine?

I guess I'd better go back and change the battleList to strings!

I'll make that change and then check back in with my results, if you guys don't mind.

Thanks!
Yup. You can pretty much guarantee that if a function returns a pointer, it's a pointer to temporary storage. There's some exceptions to that though (strdup() springs to mind).

I always found it useful to think of pointers like postal addresses and buildings. You have a block of memory allocated somewhere (Say 32 bytes inside the inet_ntoa function), and you have a much smaller (4 bytes on a 32-bit OS) pointer that stores the address of that block of memory (Much like a postal address of a few letters refers to a large building made of hundreds of bricks). When you return a pointer, you're returning the address of the chunk of memory, not a copy of it. Likewise, you can copy the pointer as much as you like, but it still points to the same address - you can have 50 envelopes addressed to the same address, you don't have 50 copies of the building.
Two calls to inet_ntoa() will cause the second call to overwrite the contents of the first. Going back to the anology, you can write a letter addressed to a building, rebuild the building, and then write another letter addressed to the same building (I.e. store a pointer to a buffer, change the buffer, and store another pointer - or call inet_ntoa(), store the result, then call inet_ntoa() again). The first letter will now be delivered to the new building that's at the same address. Likewise, after the second call to inet_ntoa(), the second pointer is the same as the first.

Anyway, I'm blithering. I'll shut up now.


EDIT:
Quote:
Original post by dphoenix
Darn, see this is a time when it would be nice to be able to use Visual Studio. By far the best debugger I've used. I don't think Code::Blocks has that functionality (though I may be wrong!)
If you're developing for Windows, I'd strongly recommend getting Visual Studio - it's free and fully functional. You can always develop code using both Visual Studio and another IDE/compiler if you want to support multiple platforms.

Share this post


Link to post
Share on other sites
You guys rocked it! I changed my const char* all to strings, and when I need to send a message I just used c_str(). It worked!

Funny thing is, Zahlman I've read that siggy of yours many times and it never really sank in till just now lol.

Steve, I actually do have Visual Studio, and I absolutely love the debugger. Only thing is for the project I'm working on, the networked code is straight from Linux libraries (I'm using Cygwin), and it won't compile on VS :( And you weren't blithering, I think you explained pointers well! I just had no idea inet_ntoa used the same address space each time...it never even occured to me, but it's like "duh" now ;)

Share this post


Link to post
Share on other sites
Quote:
Original post by dphoenix
Steve, I actually do have Visual Studio, and I absolutely love the debugger. Only thing is for the project I'm working on, the networked code is straight from Linux libraries (I'm using Cygwin), and it won't compile on VS :( And you weren't blithering, I think you explained pointers well! I just had no idea inet_ntoa used the same address space each time...it never even occured to me, but it's like "duh" now ;)
Plain BSD sockets should work fine in Visual Studio; you should just need to add calls to WSAStartup() and WSACleanup(). If you have other code though, it may be harder to port. If you can get the code to work on Visual Studio without too much tinkering, I'd highly recommend it. Visual Studio is widely used in the games industry, and it's debugger is the best you can get when developing on Windows.

There's also DevCpp which I gather has a debugger, but I've never actually used it myself, and I have no idea how well it plays with Cygwin (if at all).

Share this post


Link to post
Share on other sites
Quote:
Original post by dphoenix
You guys rocked it! I changed my const char* all to strings, and when I need to send a message I just used c_str(). It worked!

Funny thing is, Zahlman I've read that siggy of yours many times and it never really sank in till just now lol.


Yeah yeah, some people just have to push the big red button before they learn no matter what you tell them ;)

@Evil Steve: They often also return one of the parameters (if it was the same type), one of the parameters plus an offset (e.g. to indicate a position in an input string), or NULL. :)

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this