c-ridgway / cpp-feather-ini-parser

Header only, simple, fast, templated - portable INI parser for ANSI C++.
MIT License
47 stars 14 forks source link

Reporting failure to parse (instead of segfaulting) #9

Closed DomtronVox closed 5 years ago

DomtronVox commented 5 years ago

First, thanks for making this it checks the boxes that I need that the various other INI libs didn't. However, I have two issues: one functional the othor a bug.

For the functional issue

The parse function/ini class has no way of indicating if the given file is invalid/has errors. For example if a line has no '=' then that line should have an error. Current functionality has it just ignore that line.

For the bug

I did not add a Section to the file at first. To my understanding this is valid formatting for INI files where key/value pairs can come before any section declarations and will either have no section or default to the section named "" (empty string). Regardless of whether you want to support that, the current bug is that leaving off the section results in a segmentation fault as the parse function tries to store a key/value pair into a section that doesn't exist.

The ini file I made was like so:

id=2dk53l89mn3n7JdY801264Kds0H```

The relivant line of the segfault error in valgrind was:
```==9642==    by 0x4E632BA: INI<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >::parse(std::istream&) (INI.h:334)

INI.h:334 (*current)[key] = value;

Suggestions

I wasn't sure if a pull request was welcome. I'll likely try implementing the following myself in a fork if I don't hear back from you.

My suggestion is the creation of a "" Section at the beginning of parse and setting it to the "current" member variable. Possibly removing the "" Section at the end if no value pairs were added to it.

I also suggest some sort of error code int that is set to certain values when a parse fails in different ways with 0 being the default/success code and provide either direct access to the variable or a getter function.

Thank you.

DomtronVox commented 5 years ago

I wasn't able to get what I wanted working so I'm moving on to other libs or maybe writing my own. I tried to insert the following code at around line 304.

//create a blank section and set it to current if there was no section
//  declared before the first key/value pair in the file
if ( current == NULL ) 
{ 
    section_t section;
    fini_sstream_t out2;
    current = new keys_t;

    out2 << "";
    Converters::GetLine(out2, section);

    sections[section] = current;
} 

I tried to mimic how sections were created in other areas also tried to use the create method I found. The issue is that current == NULL and current == nullptr doesn't seem to detect the problem state of "current". I dug around in valgrind and GDB but couldn't figure it out. :man_shrugging:

Anyway maybe that will help someone else.

Turbine1991 commented 5 years ago

You're right, it should contain error checking.

I originally designed it to focus entirely on performance as I was processing huge amounts of data. The plan is to recreate this library explicitly using C++, with no C. Error checking will be apart of this version.

Turbine1991 commented 5 years ago

I've finally re-written this parser. Everything has been addressed.

There are no line length limits.

The following is now parsed without errors: -Comments (// and #) -Semi-colons at the end of a key value pair -Indentation

The following may be applied when saving: -Pruning empty sections/keys value pairs -Padding between section blocks -Spaces around section value -Spaces inside key value pairs -Indented key value pairs -Semicolons appended to key value pairs