besser82 / libxcrypt

Extended crypt library for descrypt, md5crypt, bcrypt, and others
GNU Lesser General Public License v2.1
189 stars 52 forks source link

errno can be set on successful use of crypt_ra with yescrypt #186

Closed jasonborden closed 4 months ago

jasonborden commented 4 months ago

When yescrypt memory requirements meet or exceed 32MiB such as using params jAT, jBT, jCT, etc... an optional code path in alloc_region will attempt to allocate hugepages. If hugepage allocation fails, a non hugepage allocation will be used instead. However at that point errno has been set from the failed hugepage mmap attempt.

I believe that either errno should be cleared after the failed hugepage mmap attempt or the man page should clarify that errno must not be checked to determine success for crypt_ra

solardiz commented 4 months ago

I think this is not a bug. The man page says: All four functions set errno when they fail. In other words, when they don't fail, they may leave errno at arbitrary value. This is pretty common for libc functions as well, where errno is only valid when some other indication of the call having failed is met. In our case, the previous paragraph starts with Upon error, crypt_rn and crypt_ra return a null pointer. so that's the condition.

The hugepage allocation failure scenario is just one special case. Even when an allocation succeeds, including a non-hugepage one, the libc calls that we make are free to set errno arbitrarily. Even more likely, errno may remain stale from before the call to crypt_ra. Checking errno without other indication of error is generally a bug, not only with this library and this call, unless indicated otherwise for a specific function.

We could reset errno to 0 just before successful return from crypt*, but then such bugs in applications would go unnoticed while only tested against newer libxcrypt.

We may add When the functions succeed, the value of errno is unspecified and must not be relied upon.

jasonborden commented 4 months ago

Ok, I misinterpreted All four functions set errno when they fail. to mean errno would indicate a failure. I'll close the issue. Thank you for your feedback!

solardiz commented 4 months ago

We may add When the functions succeed, the value of errno is unspecified and must not be relied upon.

I've just added a commit with this to #185.