hc_lockfile and hc_unlockfile failures are ignored at 9 of 17 call sites

hc_lockfile () returns -1 when it cannot take the lock, and on POSIX it always could: it returns early when the handle is NULL, and it gives up after the fcntl (F_SETLKW) retry loop when errno is anything but EINTR. Nine of the seventeen places that call it or its counterpart discard that value, so the code goes on to write into a file it does not hold.

$ grep -rn 'hc_lockfile\|hc_unlockfile' src/*.c | grep -v locking.c

debugfile.c  79    checked    if (hc_lockfile (&debugfile_ctx->fp) == -1)
debugfile.c  139   checked    if (hc_unlockfile (&debugfile_ctx->fp))
hashes.c     1659  checked    if (hc_lockfile (&fp) == -1)
hashes.c     1741  checked    if (hc_unlockfile (&fp) == -1)
logfile.c    56    ignored    hc_lockfile (&fp);
logfile.c    70    ignored    hc_unlockfile (&fp);
loopback.c   171   ignored    hc_lockfile (&loopback_ctx->fp);
loopback.c   177   ignored    hc_unlockfile (&loopback_ctx->fp);
outfile.c    567   ignored    hc_unlockfile (&outfile_ctx->fp);
outfile.c    623   checked    if (hc_lockfile (&outfile_ctx->fp) == -1)
outfile.c    657   ignored    hc_unlockfile (&outfile_ctx->fp);
potfile.c    306   ignored    hc_lockfile (&potfile_ctx->fp);
potfile.c    312   checked    if (hc_unlockfile (&potfile_ctx->fp))
potfile.c    326   ignored    if (potfile_ctx->batch_depth == 0) hc_lockfile (&potfile_ctx->fp);
potfile.c    347   checked    if (hc_unlockfile (&potfile_ctx->fp))
stdout.c     187   checked    if (hc_lockfile (&out.fp) == -1)
stdout.c     522   ignored    hc_unlockfile (&out.fp);

The if on potfile.c:326 is on batch_depth, not on the return value.

Two of the ignored ones matter more than the rest. potfile.c:306 takes the lock that guards every potfile_write_append () outside a batch, and potfile.c:326 takes the one that guards a whole batch, which is where a launch returning tens of thousands of results writes. If either fails, the lines go out unguarded and two hashcat processes sharing a potfile can interleave them.

Where the failure could go

Eight of the nine sit in a void function, which has nowhere to return a failure to:

logfile.c    56, 70     void logfile_append ()
loopback.c   171, 177   void loopback_write_append ()
outfile.c    567        void outfile_destroy ()
outfile.c    657        void outfile_write_close ()
potfile.c    306        void potfile_write_append ()
potfile.c    326        void potfile_batch_begin ()

The ninth, stdout.c:522, is inside process_stdout (), which returns int.

The checks that exist disagree with each other

Three shapes are in the tree at once:

if (hc_lockfile (&fp) == -1)          // hashes.c, outfile.c, stdout.c, debugfile.c:79
if (hc_unlockfile (&potfile_ctx->fp)) // debugfile.c:139, potfile.c:312, potfile.c:347
hc_lockfile (&fp);                    // the nine above

CONTRIBUTING.md asks for the first one:

$ sed -n 71p CONTRIBUTING.md
* Compare explicitly: `if (found == true)`, `if (len == 0)`. Never `if (!found)`.

What a failure should do is not settled either. hashes.c, outfile.c and stdout.c close the file, report strerror (errno) and return -1. debugfile.c and potfile.c print a fixed message and carry on.

Why it is worth saying now

#4646 implements these two functions on Windows, where they are currently stubs that return 0. That does not create this problem: on POSIX the same nine sites have always ignored the same failures. It does remove the reason one could say the value never means anything, because until now on Windows it genuinely did not.

I left the callers out of #4646 deliberately: deciding what each of them should do on a failed lock is a separate question from implementing the lock, and eight sites have been given checks already, in two different shapes, so I did not want to add a third for the rest.

Happy to do the work if you say which way you want it: == -1 everywhere with the existing message style, or the close-and-fail shape where the function can return an error and a message where it cannot.