Re: mandocdb tools, sqlite3, and ohash

[Ingo Schwarze <schwarze@usta.de>]

Hi Kristaps,

Kristaps Dzonsons wrote on Thu, Jun 07, 2012 at 05:15:24PM +0200:

> I've now kicked out uthash in favour of OpenBSD's native ohash.

Thanks for doing that work!
That removes one issue preventing a merge.

> (I've included espie@ in this just in case he has any ohashy
> wisdom.)  This requires that I add a compat_ohash.c file, but this
> is trivial.

Marc, i have left you in the Cc: such that in case you test,
there is less risk of duplicating work.  Just ignore this mail
if you don't want to test at this early stage, and say so if
you want to be removed from the Cc: for stuff not directly
related to ohash.


So far, i have read the file mandocdb.c and found the following
potential issues.  I'm calling them "potential" because this is
mostly coming from code inspection without much testing.

 1. Function main, OP_UPDATE case:
    Is calling ofmerge, i.e. dbopen with real=0, correct?
    It calls dbclose with real=0, thus calling rename to MANDOC_DB,
    thus clobbering the original database instead of adding to it?

    So far, i cannot really test this suspicion
    because mandocdb -d always dies with segfaults or floating
    point exceptions for me, i would have to ivestigate those
    crashes first.

 2. Function main, rebuild case:
    Shouldn't this create the new db first, then move it into place
    atomically in case of success?  The remove(MANDOC_DB) seems
    problematic:  It is deleting an existing database, then creating
    a new one only considerably later, if at all.

 3. Function main, with multiple relative directories:  e.g.
      # cd /usr; mandocdb share/man local/man
    That causes the first chdir to change into /usr/share/man
    and the second to try to change into /usr/share/man/local/man.
    That may not be a new issue, though.

 4. Function dbopen with real=0:
    There seems to be a race, open to symlink attacks,
    between remove(file) and sqlite3_open(file);
    some kind of O_EXCL or something might be required.

 5. Function dirscan:
    Shouldn't an argc argument be int instead of size_t?

 6. Functions dirtreescan and filescan:
    When a manual page has multiple hard links (MLINKS),
    it looks like a matter of chance which one ends up being
    used.  Is there any check whether the NAME agrees with
    the file name?  Not sure anything is broken here, just
    wondering.

 7. Function filescan:
    Calling mandocdb -d with file argument(s) triggers
    the use_all assertion.

 8. Function inocheck:
    The memset(&id seems pointless, values are assigned
    to the members right afterwards.

 9. Function parse_mdoc_Fd:
    This function triggers an assertion on #include <>.
    Admittedly, that's not a new problem.
    I'm not fixing it right now to avoid merge conflicts,
    but we shouldn't forget.

Yours,
  Ingo
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv