Re: mandocdb tools, sqlite3, and ohash

[Kristaps Dzonsons <kristaps@bsd.lv>]

> 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.

Ingo,

Thanks for looking this over!  I'll commit this code in bsd.lv with the 
following changes so we can stop throwing huge diffs back and forth.

I hadn't tested the -d, -u, and -t since using ohash.  I now see that I 
hadn't initialised the hashes in the right place (thus, FPE and 
segfaults from using unitialised or zeroed data).  Fixed.

>   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?

Yes, that was ugly.  I moved dbopen() into main() where it belongs 
(along with the matching dbclose)).

>      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.

Also fixed by removing the remove() entirely---dbclose() does its own 
rename, so no need to remove.

>   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.

Oops, I thought I was handling that correctly.  This is now fixed by 
having the main() loop invoke path_reset(), which jumps into the cwd 
then into the base dir, before any relative-path operations.  This 
cleaned up a nice bit of code, I think.

>   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.

Fixed---good point.  Pity there's no O_TRUNC for sqlite3.

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

Also fixed.  Well, fixed because dirscan() is no more, now that I've 
moved the fchdir()/chdir() into reset_path().  The filescan() loop is 
now right in main().

>   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.

Nope.  And that'd be a little tricksy given that a single file can have 
many NAMEs.

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

Fixed (-d, -u, and -t all raise use_all).

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

It's not pointless at all.  That ohash_lookup_memory() is using the 
struct address and size, which may consist of padding.  memset() zeroes 
the padding bytes, otherwise we might lookup based on random values in 
the padding.

>   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.

Fixed anyway.

Thanks again, Ingo!  I really appreciate it!

Best,

Kristaps
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv