Skip to content

Safe-guard the Windows code a bit more against getenv() problems #127

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Safe-guard the Windows code a bit more against getenv() problems #127

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compat/mingw.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1632,7 +1632,7 @@ int mingw_kill(pid_t pid, int sig)
*/
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Jeff King wrote (reply to this):

On Fri, Feb 15, 2019 at 07:17:45AM -0800, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> Running up to v2.21.0, we fixed two bugs that were made prominent by the
> Windows-specific change to retain copies of only the 30 latest getenv()
> calls' returned strings, invalidating any copies of previous getenv()
> calls' return values.
> 
> While this really shines a light onto bugs of the form where we hold
> onto getenv()'s return values without copying them, it is also a real
> problem for users.
> 
> And even if Jeff King's patches merged via 773e408881 (Merge branch
> 'jk/save-getenv-result', 2019-01-29) provide further work on that front,
> we are far from done. Just one example: on Windows, we unset environment
> variables when spawning new processes, which potentially invalidates
> strings that were previously obtained via getenv(), and therefore we
> have to duplicate environment values that are somehow involved in
> spawning new processes (e.g. GIT_MAN_VIEWER in show_man_page()).

Belated review, as this is already in master, but: yes, I absolutely
support this patch, even on top of my patches. Those were just the cases
I found by poking around for a few minutes, and I'm sure there are many
more.

-Peff

char *mingw_getenv(const char *name)
{
#define GETENV_MAX_RETAIN 30
#define GETENV_MAX_RETAIN 64
static char *values[GETENV_MAX_RETAIN];
static int value_counter;
int len_key, len_value;
Expand Down