Re: [PATCH 2/3] strbuf: set errno to 0 after strbuf_getcwd
- From
Kyle Lippincott <spectral@google.com>
- Date
- Aug 2, 2024, 17:56 UTC
- Message-ID
- <CAO_smVh-16cfWDOq_XwNHpov7coufu-m-buexz86+MBYnFb3YA@mail.gmail.com>
- In-Reply-To
- <xmqqv80jeza5.fsf@gitster.g>
On Fri, Aug 2, 2024 at 8:10 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 27 quoted lines
>
> "Kyle Lippincott via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > Set `errno = 0;` prior to exiting from `strbuf_getcwd` successfully.
> > This matches the behavior in functions like `run_transaction_hook`
> > (refs.c:2176) and `read_ref_internal` (refs/files-backend.c:564).
>
> This deep in the call chain, there is nothing that assures us that
> the caller of this function does not care about the error before
> entering this function, so I feel a bit uneasy about the approach,
> and my initial reaction was "wouldn't it be safer to do the usual
>
> int saved_errno = errno;
>
> for (guessed_len = 128;; guessed_len *= 2) {
> ... do things ...
> if (...) {
> ... happy ...
> errno = saved_errno;
> return 0;
> }
> }
>
> pattern.
>
> Who calls this function, and inspects errno when this function
> returns 0?That's a difficult question to answer if you want to be wholistic for the whole program :) For immediate callers: - unix_sockaddr_init: doesn't inspect or adjust errno itself if strbuf_getcwd returns 0. Continues on to call other functions that may set errno. - strbuf_realpath_1: same - chdir_notify: same - discover_git_directory_reason: same - setup_git_directory_gently: same - setup_enlistment_directory (in scalar.c): dies immediately if strbuf_getcwd returns < 0, otherwise same - xgetcwd: also doesn't inspect/adjust errno if strbuf_getcwd returns 0. Doesn't call any other functions afterward (besides strbuf functions). - main: stores the value if strbuf_getcwd returns 0, but doesn't inspect errno.
> I do not mind adding the "save and restore" fix to this > function, but if there is a caller that looks at errno from a call > that returns success, that caller may also have to be looked at and > fixed if necessary.
There aren't any that I could find, this patch is mostly a defense-in-depth solution to the strtoX functions that were fixed in patch 1. This function _may_ set errno even on success. That errno value ends up retained indefinitely as long as things continue succeeding, and then we call a function like `strtod` which has a suboptimal interface. If this patch doesn't land, the codebase is still correct; the main reason to want to land this is that without this patch, any user that has paths longer than 128 bytes becomes de facto responsible for finding and reporting/fixing issues that arise from this errno value being persisted, and I was hoping I wouldn't be signing the people maintaining CI at $JOB up for that :) It's not an obvious failure, either. For example, t0211's failure, prior to setting errno to 0 just before calling strtoX is just: `fatal: expect <exit_code>`. That's not easy to trace back to "strbuf_getcwd sets ERANGE in errno in our environment, so this is a misuse of a strtoX or parse_timestamp function".
Show 19 quoted lines
>
> Thanks.
>
> > Signed-off-by: Kyle Lippincott <spectral@google.com>
> > ---
> > strbuf.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/strbuf.c b/strbuf.c
> > index 3d2189a7f64..b94ef040ab0 100644
> > --- a/strbuf.c
> > +++ b/strbuf.c
> > @@ -601,6 +601,7 @@ int strbuf_getcwd(struct strbuf *sb)
> > strbuf_grow(sb, guessed_len);
> > if (getcwd(sb->buf, sb->alloc)) {
> > strbuf_setlen(sb, strlen(sb->buf));
> > + errno = 0;
> > return 0;
> > }