git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] wrapper: Fix a errno discrepancy on NetBSD.

From
shejialuo <shejialuo@gmail.com>
Date
May 6, 2025, 13:43 UTC
Message-ID
<aBoR8hrcpK6CzbA3@ArchLinux>
In-Reply-To
<20250505180311.GA29783@coredump.intra.peff.net>
On Mon, May 05, 2025 at 02:03:11PM -0400, Jeff King wrote:
Show 37 quoted lines
> On Mon, May 05, 2025 at 08:43:18AM -0700, Junio C Hamano wrote:
> 
> > But for other kind of requirements, we want to fulfill them on all
> > platforms that we claim to support.  Using open_nofollow() to
> > achieve hard atomicity requirement would be a bug in such a
> > situation.  Should we somehow warn our developers against its use?
> 
> The comment above the declaration says:
> 
>   /*
>    * Open with O_NOFOLLOW, or equivalent. Note that the fallback equivalent
>    * may be racy. Do not use this as protection against an attacker who can
>    * simultaneously create paths.
>    */
>   int open_nofollow(const char *path, int flags);
> 
> though that may not be enough. 00611d8440 (add open_nofollow() helper,
> 2021-02-16) discusses a way that it could be made less racy, at a
> slightly increased cost.
> 
> IMHO that is somewhat orthogonal to the issue here, though, which is
> purely about the case where O_NOFOLLOW does exist (ironically, our
> racy fallback code consistently returns ELOOP ;) ).
> 
> The issue at hand is that particular errno responses are not always
> portable. The patch discussed here improves that. My point was more that
> I'm not sure to what degree we should care about errno consistency in
> our wrappers (which is inherently a bit whack-a-mole as we find new
> cases), versus trying not to care too hard about specific errno values
> in calling code.
> 
> I can see arguments either way (and as I said, an argument for making
> errno values consistent even if we try to rely on them less). Mostly I
> was just a little surprised to see open_nofollow() being used in this
> way (especially since we have to end up stat()-ing anyway to check for
> other cases).
> 

IIRC, we wanted to try our best to make our code consistent. In the very early implementation, I actually firstly checked the file type and then opened the file.

However, there is a chance that the raw "packed-refs" file could be converted to symlink between checking the filetype and opening the file to get the fd. Although, in fsck, we may just ignore this. But during the review, I found out that using "open_nofollow" could avoid race in some platforms. Sadly, I haven't realized that this would break compatibility ;)

Because using "open_nofollow" could only check whether the filetype is the symlink, we also need to use "stat" again to check whether the filetype is OK. I agree that it is a little redundant.

Since the patch from Collin would solve the problem. I won't change the logic. I'll focus on using `mmap` to open the "packed-refs" file.

> -Peff

Thanks, Jialuo

Previous: Jeff KingNext: Junio C Hamano
Message 15 of 25 in “wrapper: Fix a errno discrepancy on NetBSD.”
  1. wrapper: Fix a errno discrepancy on NetBSD.Collin Funk, May 2, 2025
  2. brian m. carlsonMay 3, 2025
  3. Junio C HamanoMay 3, 2025
  4. Collin FunkMay 3, 2025
  5. Junio C HamanoMay 3, 2025
  6. Collin FunkMay 3, 2025
  7. Jeff KingMay 3, 2025
  8. shejialuoMay 3, 2025
  9. Jeff KingMay 3, 2025
  10. Patrick SteinhardtMay 5, 2025
  11. shejialuoMay 5, 2025
  12. Collin FunkMay 3, 2025
  13. Junio C HamanoMay 5, 2025
  14. Jeff KingMay 5, 2025
  15. shejialuoMay 6, 2025
  16. Junio C HamanoMay 6, 2025
  17. wrapper: NetBSD gives EFTYPE and FreeBSD gives EMFILE where POSIX uses ELOOPCollin Funk, May 3, 2025
  18. brian m. carlsonMay 3, 2025
  19. Collin FunkMay 3, 2025
  20. Patrick SteinhardtMay 5, 2025
  21. Junio C HamanoMay 5, 2025
  22. Collin FunkMay 6, 2025
  23. Patrick SteinhardtMay 6, 2025
  24. wrapper: NetBSD gives EFTYPE and FreeBSD gives EMFILE where POSIX uses ELOOPCollin Funk, May 6, 2025
  25. Patrick SteinhardtMay 6, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.