On Fri, Aug 2, 2024 at 8:10 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> "Kyle Lippincott via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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".
>
> Thanks.
>
> > Signed-off-by: Kyle Lippincott <spectral@xxxxxxxxxx>
> > ---
> > 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;
> > }
