opencog / link-grammar

The CMU Link Grammar natural language parser
GNU Lesser General Public License v2.1
389 stars 119 forks source link

Avoid core dump from some bad pointer math. #1522

Closed linas closed 6 months ago

linas commented 6 months ago

This is a temporary patch, till I figure out something better.

ampli commented 6 months ago

Instead, why not:

if (0 != dict->num_categories)
{
   prt_error("Warning: Dictionary %s has no categories\n";
   return NULL;
}

(Not tested.)

linas commented 6 months ago

I tried that. Returning null here causes a crash later on, when no expressions are found. I figured that the earlier the catch, the better.

ampli commented 6 months ago

I proposed a warning because I try to keep assert() for logic failures (as opposed to bad external data). Otherwise (in the general case), a program may crash when it could just continue to perform fine.

In this specific case, I could think about two such scenarios (to demonstrate the idea of not crashing on bad data):

  1. A thread with a bad dictionary (maybe created on the fly) is added, crashing a long-running program with other good threads.
  2. A hypothetical interactive program in which the user creates dictionaries and accidentally creates a dictionary with no categories. Crashing may abort other interactive work that the user has done.

However, if the program encounters bad logic, it means there is a bug that may, e.g., clobber memory and there may be no point in continuing. BTW, even in such a case, there may be a benefit to not immediately aborting, e.g., if an editor program uses the library and may want to save data before aborting. To that end, I created lg_library_failure_hook (not tested).

linas commented 6 months ago

This was meant to be temporary. The failure_hook thing seems too complicated; I would not want to use it, at least, not right now.

The easiest solution would be to just look at the number of categories in the dict, when the dict is opened, and print "can't use this for generation", and then immediately return zero results. (and then the assert can be removed) I was in too much of a hurry to figure out where that would need to be done.

The assert leaves me with an error message, which is still better than having to run gdb to find out what happened.