crypt-(gensalt-)static: Do not overwrite the output buffer too early.

[besser82]

Allow passing a pointer returned by crypt(3) previously as the input for a subsequent call to crypt(3), adapt the manpage accordingly, and implement a simple testcase for such a scenario.

Also apply the same semantics to crypt_gensalt(3).

Fixes #209.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.46%. Comparing base (e34e063) to head (962ad88).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #210      +/-   ##
===========================================
+ Coverage    90.44%   90.46%   +0.01%     
===========================================
  Files           36       36              
  Lines         3988     3995       +7     
  Branches       747      749       +2     
===========================================
+ Hits          3607     3614       +7     
  Misses         242      242              
  Partials       139      139              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[solardiz]

Thanks! I agree with the man page edits, but disagree with the code change. Let's not special-case this. There are other related cases to take care of as well: passing the previous return value as key rather than setting, and passing a pointer into the middle of a previous return value (e.g., skip over the original salt when chaining multiple hash types).

[solardiz]

To be clear, I do not mean to also special-case those other cases - rather, to develop a generic fix for all of these that would not need to check any conditions. Just don't overwrite the output buffer before reading both inputs, ever.

[besser82]

To be clear, I do not mean to also special-case those other cases - rather, to develop a generic fix for all of these that would not need to check any conditions. Just don't overwrite the output buffer before reading both inputs, ever.

Should be addressed properly in the rebased commit.

[besser82]

@solardiz Is everything okay with the changes now?

[solardiz]

The changes look OK to me now. However, I am concerned about a few related things (I'll write a separate comment).

[solardiz]

I'm sorry it took me a while to get to reviewing this again. It's hard for me to make time when I can concentrate on critical code reviews now.

I'm concerned that we're adding complexity to paper over prior code's shortcomings, but OTOH I understand that we care not only about risk of bugs in latest code, but about risk of bugs across all versions, and more code changes mean increasing the latter risk. That's an unfortunate tradeoff.

For brand new code, we'd probably postpone the make_failure_token calls until we reach an error path.

Please don't make any changes to address this comment yet. Let's discuss first, along with the additional concern I just brought up in #209. Maybe @ldv-alt would want to weigh in?