Server crashes when advanced and field searching combined in MGPPcol

[ak19]

Tested with an up to date GS2:

• server.exe and apache-httpd server, Windows. It's not just there in the server.exe + demo coll exported to CD (where it was first discovered), but the bug is already there in a normal GS install's server.exe and apache-httpd.

• MGPP collection "Demo".

• Advanced searching (boolean) AND Fielded searching turned on:

• Bug occurs whenever the 2nd, 3rd AND/OR 4th search fields are filled in in the search matter, no matter which indexed fields are chosen to search over for each of these search fields. The bug occurs REGARDLESS of whether the 1st search field is filled in.

• 1st search field works when no other search fields are filled in, with or without boolean operators specified in the search field and regardless of what the indexed field of the first search field is that we're searching in.

• server.exe crashes when such a search is run. When using apache-httpd, the dialog says that it is library.cgi that has crashed.

[ak19]

The above tests were conducted with an svn checkout made up to date (not a fresh checkout, but a proper clean + recompile was conducted).

The apache web server of the Linux binary from yesterday's caveat, 14 Sept, works fine. Will try with a fresh GS2 checkout on Windows.

[ak19]

Fresh GS2 checkout from svn on Windows: same problem with both server.exe and the apache web server.

[ak19]

The problem is there in Linux and Windows but due to circumstance doesn't result in a crash on Linux. There are two related bugs:

1. parse_adv_query_form() function of runtime-src\src\recpt\querytools.cpp

CGI args stems and folds in this function are not delimited by commas but by the URL-encoded value for comma (%2C), and splitting them into arrays around the comma character was therefore ineffective. This led to arrays containing fewer indexed values than the program was expecting to deal with and illegal array index access caused the first crash.

The solution was to call the cgiutils.cpp method decode_commas()

2. decode_commas() function of runtime-src\src\recpt\cgiutils.cpp

The second crash was caused by decode_commas() using the STL iterator addition to increment well past the end of the iterator when trying to read %2C and write , values in their place.

According to inspecting the STL code, + and += overloaded operators are specifically not allowed to take operands that would put the iterator past the end of the sequence of values it is iterating over. In this case, the (iterator+2) > end test causes a crash when the +2 operation results in an illegal iterator index (i.e. when it IS past 'end').

The solution was to increment one character at a time when using the iterator and checking that it's != end. Testing against 'end' itself is an allowed test.

Still need to expand the solution of 1 to work with parsing arguments given for an SQL query and find the right location to do the URL %2C conversion to comma.

The above fixes so far are in querytools.cpp and cgiutils.cpp of runtime-src\src\recpt and are committed under revision numbers 22934 and 22935.

[ak19]

Related SQL query fixes:

Dr Bainbridge fixed more bugs tangentially related to the advanced searching combined with fielded searching bug that was previously crashing the library.cgi and server.exe. This time around, there were no crashes but the recent corrections to the combined searching needed to work with SQL queries as well. When testing this last, it was discovered that 1. the SQL form wasn't displaying in preferences even when sqlite was set as the infodbtype in collect.cfg and its format Searchtypes line set to include sqlform next to plain and form. This was fixed by making CGIWrapper remember to instantiate sqlqueryaction and not just queryaction. 2. the RunQuery button would eat commas on submission (previously choosing to turn them into non-descript spaces), this has now been fixed in the query.dm macro.

Required changes to the following files, fixed with the commits for revision 22948:

• runtime-src\src\w32server\cgiwrapper.cpp

• macros\query.dm

[ak19]

1. Undoing commit of 22934 where decode_commas was called on stem and fold comma separated list: previously separated due to url-encoding of commas. Now that the problem has been fixed at the source, the decode_commas hack is no longer necessary.

2. Commas in stem and fold are no longer url-encoded because the multiple_value field of the continuously-reused struct arg_ainfo is always set back to the default false after ever being set to true. So it no longer subtly stays at true to affect Greenstone functioning in unforeseen ways (such as suddenly and unnecessarily URL-encoding commas where this is not wanted)."

Changes committed to runtime-src\src\recpt with revision 22984 for the above:

1. querytools.cpp

2. authenaction.cpp, basequeryaction.cpp, browseaction.cpp, collectoraction.cpp

configaction.cpp, depositoraction.cpp, documentaction.cpp, dynamicclassifieraction.cpp, extlinkaction.cpp, gtiaction.cpp, pageaction.cpp phindaction.cpp, pingaction.cpp, queryaction.cpp, sqlqueryaction.cpp statusaction.cpp, tipaction.cpp, usersaction.cpp