[2/3] strbuf: set errno to 0 after strbuf_getcwd

Commit Message

Kyle Lippincott Aug. 2, 2024, 4:10 a.m. UTC

From: Kyle Lippincott <[e-mail]>

If the loop executes more than once due to cwd being longer than 128
bytes, then `errno = ERANGE` might persist outside of this function.
This technically shouldn't be a problem, as all locations where the
value in `errno` is tested should either (a) call a function that's
guaranteed to set `errno` to 0 on success, or (b) set `errno` to 0 prior
to calling the function that only conditionally sets errno, such as the
`strtod` function. In the case of functions in category (b), it's easy
to forget to do that.

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).

Signed-off-by: Kyle Lippincott <[e-mail]>
---
 strbuf.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Junio C Hamano Aug. 2, 2024, 3:10 p.m. UTC | #1

"Kyle Lippincott via GitGitGadget" <[e-mail]> 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?  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.

Thanks.

> Signed-off-by: Kyle Lippincott <[e-mail]>
> ---
>  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;
>  		}

Kyle Lippincott Aug. 2, 2024, 5:56 p.m. UTC | #2

On Fri, Aug 2, 2024 at 8:10 AM Junio C Hamano <[e-mail]> wrote:
>
> "Kyle Lippincott via GitGitGadget" <[e-mail]> 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 <[e-mail]>
> > ---
> >  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;
> >               }

Patch

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;
 		}