Thread: Hola

  1. #1
    Registered User xentaka's Avatar
    Join Date
    May 2011
    Posts
    60

    Hola

    I am new to C++ and the boards so just wanted to drop in and say hi before I start assaulting you with stupid questions.

    Actually I just started my first real project ( parsing a firewall log ) and everything was going spectacularly until I threw some bad strings at it.... crash n burn.

    Was using the string.find() and string.assign() to do the majority of my parsing, but I think what crashed was the atoi() conversions to test IP addys and dates. Anyway it was pretty awesome cause even though I failed it was a great learning experience.

    Sure I will be back with questions soon enough.

    EDIT: Here's the code that is failing I think... pretty sure gonna rewrite whole program though.

    EDIT EDIT: Should mention if the string is constructed properly this works great, but throw some bad data and it goes poof and crashes.

    Code:
    int parseTXT::checkDate(string checkD) {
        int chkYear, chkMonth, firstD, secondD, chkDay;
        firstD = checkD.find("-",0);
        secondD = checkD.find("-", (firstD + 1));
        chkYear = atoi((checkD.assign(checkD.begin()+0, checkD.begin()+firstD)).c_str());
        chkMonth = atoi((checkD.assign(checkD.begin()+(secondD - 2), checkD.begin()+secondD)).c_str());
        chkDay = atoi((checkD.assign(checkD.begin()+(secondD + 1), checkD.begin()+(secondD + 3))).c_str());
    
        if ((chkYear < 1990) || (chkYear > 2100)) {
            return 0;
        }
        if ((chkDay <= 0) || (chkDay > 31)) {
            return 0;
        }
        if ((chkMonth <= 0) || (chkMonth > 12)) {
            return 0;
        }
        return 1;
    }
    Last edited by xentaka; 05-19-2011 at 02:59 PM.

  2. #2
    Unregistered User Yarin's Avatar
    Join Date
    Jul 2007
    Posts
    2,158
    You need to learn to error check. For example, the first thing that popped out is that when you find() a dash character, you don't check the return value for a possible -1 (which means, none could be found), and because your code ignores that possibility, you could get undefined behavior, which doesn't get along with reality very well.

  3. #3
    Registered User
    Join Date
    Oct 2008
    Posts
    1,262
    Welcome to the forums!

    There are two problems with your code I can see right now:
    1. You don't check if "find" actually found the dash. If not, it will crash.
    2. You overwrite checkD by calling checkD.assign, invalidating everything you found before. Don't use assign - it's useless anyway. Use .substr to do this.

    So even if you throw it a valid string this piece of code may very well crash...


    Edit: Yarin beat me to it for 1. But does it actually return -1? You should check whether the value is std::string::npos, not -1. Maybe it's -1 on most systems, but I doubt it is defined like that in the standard.

  4. #4
    Registered User xentaka's Avatar
    Join Date
    May 2011
    Posts
    60
    Quote Originally Posted by Yarin View Post
    You need to learn to error check. For example, the first thing that popped out is that when you find() a dash character, you don't check the return value for a possible -1 (which means, none could be found), and because your code ignores that possibility, you could get undefined behavior, which doesn't get along with reality very well.
    I should have used a try/catch system, but this was written quickly to test the validity of using such a system. I see what you mean though... so like:

    Code:
    if(checkD.find("-",0) == -1) {
    cout << endl << "Malformed String!" << endl;
    return 0; // error return in main
    } else {
    firstD = checkD.find("-",0);
    }
    EDIT: Works perfectly with a properly formed string EVOEx... just badly with a malformed one.

    Just realized if I nipped the malform with early error detection this could still work! Just discard malformed lines!
    Thanks guys.

    Also reading about substr now... that really is what i should be using.
    Last edited by xentaka; 05-19-2011 at 03:41 PM.

  5. #5
    Registered User
    Join Date
    Dec 2007
    Posts
    2,675
    In addition, where you're using C++, you should avoid using atoi() and opt for the stringstream class.

  6. #6
    Unregistered User Yarin's Avatar
    Join Date
    Jul 2007
    Posts
    2,158
    Quote Originally Posted by EVOEx View Post
    But does it actually return -1? You should check whether the value is std::string::npos, not -1. Maybe it's -1 on most systems, but I doubt it is defined like that in the standard.
    Yes, you're right, my mistake. (A somewhat bad habit of mine, I guess )

  7. #7
    Registered User xentaka's Avatar
    Join Date
    May 2011
    Posts
    60

    Rewriting as we speak.

    Doing a pretty massive rewrite as we speak...

    1. Changing all the assign() to substr() - This turned out to be a bit of a beast since I had to rewrite due to substr wanting pos,length.

    2. Changed the atoi functionality over to stringstream.

    3. Working on error control to stop the crashing due to malformed strings.

    EDIT: No error checking yet, but this is an example of changes made.
    *posted wrong code* woops
    Last edited by xentaka; 05-20-2011 at 01:04 AM.

  8. #8
    Registered User xentaka's Avatar
    Join Date
    May 2011
    Posts
    60
    Just an update...

    Got the program fully functional thanks to you guys! I can give it a complete garbage file and it will just put up error messages and no crashes.

    If I give it a file with parts from the line missing, and it will throw errors for the part of the line that isn't there but will keep parsing the parts that are there. I absolutely adore my new creation and now only need to make a windows interface for it.

    EDIT: This is the updated snippet that I posted earlier.

    Code:
    int parseTXT::checkDate(string checkD) {
        if (checkD.find("ERROR",0) == string::npos) {
            int chkYear, chkMonth, firstD, secondD, chkDay, days1, month1;
            if (checkD.find("-",0) != string::npos) {
                firstD = checkD.find("-",0);
            } else { return 0; }
            if (checkD.find("-", (firstD +1)) != string::npos) {
                secondD = checkD.find("-", (firstD + 1));
            } else { return 0; }
            month1 = firstD + 1;
            days1 = secondD + 1;
            stringstream(checkD.substr(0, 4)) >> chkYear;
            stringstream(checkD.substr(days1, 2)) >> chkDay;
            stringstream(checkD.substr(month1, 2)) >> chkMonth;
    
            if ((chkYear < 1990) || (chkYear > 2100)) {
                return 0;
            }
            if ((chkDay <= 0) || (chkDay > 31)) {
                return 0;
            }
            if ((chkMonth <= 0) || (chkMonth > 12)) {
                return 0;
            }
            return 1;
        }
        return 0;
    }
    Last edited by xentaka; 05-20-2011 at 01:01 AM.

  9. #9
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Location
    Inside my computer
    Posts
    24,654
    firstD and secondD may be std::string::npos.
    The extracted substring may not be what you expect it to be, so the extraction may fail.
    You should check for these things when it comes to parsing. Otherwise you will have your code fail or crash at malformed lines.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  10. #10
    Registered User xentaka's Avatar
    Join Date
    May 2011
    Posts
    60

    What about...

    What if I'm :

    Code:
    using namespace std;
    I realize it's not the best thing to do, but my program is quite small and it's very convenient.

  11. #11
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Location
    Inside my computer
    Posts
    24,654
    That will have no impact on the logic of your code (hopefully). Whether it's a good idea or not is debatable. It's really up to you, if you want to do it or not.
    It increases the chances of name collisions, but it means less typing.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Hola!
    By correlcj in forum C++ Programming
    Replies: 4
    Last Post: 10-03-2002, 03:27 PM