| Message ID | 3d92528125ee419aefdac790dc1a4106be632c60.1717402403.git.ps@pks.im (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,01/27] global: improve const correctness when assigning string constants | expand |
On Mon, Jun 3, 2024 at 5:46 AM Patrick Steinhardt <ps@pks.im> wrote: > Adjust various places in our Win32 compatibility layer where we are not > assigning string constants to `const char *` variables. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > diff --git a/compat/basename.c b/compat/basename.c > @@ -1,6 +1,8 @@ > +static char current_directory[] = "."; > @@ -10,7 +12,13 @@ char *gitbasename (char *path) > if (!path || !*path) > - return "."; > + /* > + * basename(3P) is mis-specified because it returns a > + * non-constant pointer even though it is specified to return a > + * pointer to internal memory at times. The cast is a result of > + * that. > + */ > + return (char *) ""; The change from returning "." to returning "" is unexplained by the commit message. Did you mean to return the newly-introduced `current_directory` instead? > @@ -34,7 +42,13 @@ char *gitdirname(char *path) > if (!p) > - return "."; > + /* > + * dirname(3P) is mis-specified because it returns a > + * non-constant pointer even though it is specified to return a > + * pointer to internal memory at times. The cast is a result of > + * that. > + */ > + return (char *) ""; Ditto.
Eric Sunshine <sunshine@sunshineco.com> writes: > On Mon, Jun 3, 2024 at 5:46 AM Patrick Steinhardt <ps@pks.im> wrote: >> Adjust various places in our Win32 compatibility layer where we are not >> assigning string constants to `const char *` variables. >> >> Signed-off-by: Patrick Steinhardt <ps@pks.im> >> --- >> diff --git a/compat/basename.c b/compat/basename.c >> @@ -1,6 +1,8 @@ >> +static char current_directory[] = "."; >> @@ -10,7 +12,13 @@ char *gitbasename (char *path) >> if (!path || !*path) >> - return "."; >> + /* >> + * basename(3P) is mis-specified because it returns a >> + * non-constant pointer even though it is specified to return a >> + * pointer to internal memory at times. The cast is a result of >> + * that. >> + */ >> + return (char *) ""; > > The change from returning "." to returning "" is unexplained by the > commit message. Did you mean to return the newly-introduced > `current_directory` instead? I suspect that these places wanted to return (char *) "." instead, without introducing a new variable.
On Mon, Jun 03, 2024 at 12:04:37PM -0700, Junio C Hamano wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > > On Mon, Jun 3, 2024 at 5:46 AM Patrick Steinhardt <ps@pks.im> wrote: > >> Adjust various places in our Win32 compatibility layer where we are not > >> assigning string constants to `const char *` variables. > >> > >> Signed-off-by: Patrick Steinhardt <ps@pks.im> > >> --- > >> diff --git a/compat/basename.c b/compat/basename.c > >> @@ -1,6 +1,8 @@ > >> +static char current_directory[] = "."; > >> @@ -10,7 +12,13 @@ char *gitbasename (char *path) > >> if (!path || !*path) > >> - return "."; > >> + /* > >> + * basename(3P) is mis-specified because it returns a > >> + * non-constant pointer even though it is specified to return a > >> + * pointer to internal memory at times. The cast is a result of > >> + * that. > >> + */ > >> + return (char *) ""; > > > > The change from returning "." to returning "" is unexplained by the > > commit message. Did you mean to return the newly-introduced > > `current_directory` instead? > > I suspect that these places wanted to return (char *) "." instead, > without introducing a new variable. Oof, that's bad. Thanks for catching this! Patrick
diff --git a/compat/basename.c b/compat/basename.c index 96bd9533b4..5aa320306b 100644 --- a/compat/basename.c +++ b/compat/basename.c @@ -1,6 +1,8 @@ #include "../git-compat-util.h" #include "../strbuf.h" +static char current_directory[] = "."; + /* Adapted from libiberty's basename.c. */ char *gitbasename (char *path) { @@ -10,7 +12,13 @@ char *gitbasename (char *path) skip_dos_drive_prefix(&path); if (!path || !*path) - return "."; + /* + * basename(3P) is mis-specified because it returns a + * non-constant pointer even though it is specified to return a + * pointer to internal memory at times. The cast is a result of + * that. + */ + return (char *) ""; for (base = path; *path; path++) { if (!is_dir_sep(*path)) @@ -34,7 +42,13 @@ char *gitdirname(char *path) int dos_drive_prefix; if (!p) - return "."; + /* + * dirname(3P) is mis-specified because it returns a + * non-constant pointer even though it is specified to return a + * pointer to internal memory at times. The cast is a result of + * that. + */ + return (char *) ""; if ((dos_drive_prefix = skip_dos_drive_prefix(&p)) && !*p) goto dot; diff --git a/compat/mingw.c b/compat/mingw.c index 6b06ea540f..d378cd04cb 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -2279,7 +2279,11 @@ struct passwd *getpwuid(int uid) p->pw_name = user_name; p->pw_gecos = get_extended_user_info(NameDisplay); if (!p->pw_gecos) - p->pw_gecos = "unknown"; + /* + * Data returned by getpwuid(3P) is treated as internal and + * must never be written to or freed. + */ + p->pw_gecos = (char *) "unknown"; p->pw_dir = NULL; initialized = 1; @@ -2800,16 +2804,16 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report) strbuf_addf(report, "'%s' is on a file system that does " "not record ownership\n", path); } else if (report) { - LPSTR str1, str2, str3, str4, to_free1 = NULL, - to_free3 = NULL, to_local_free2 = NULL, - to_local_free4 = NULL; + PCSTR str1, str2, str3, str4; + LPSTR to_free1 = NULL, to_free3 = NULL, + to_local_free2 = NULL, to_local_free4 = NULL; - if (user_sid_to_user_name(sid, &str1)) - to_free1 = str1; + if (user_sid_to_user_name(sid, &to_free1)) + str1 = to_free1; else str1 = "(inconvertible)"; - if (ConvertSidToStringSidA(sid, &str2)) - to_local_free2 = str2; + if (ConvertSidToStringSidA(sid, &to_local_free2)) + str2 = to_local_free2; else str2 = "(inconvertible)"; @@ -2822,13 +2826,13 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report) str4 = "(invalid)"; } else { if (user_sid_to_user_name(current_user_sid, - &str3)) - to_free3 = str3; + &to_free3)) + str3 = to_free3; else str3 = "(inconvertible)"; if (ConvertSidToStringSidA(current_user_sid, - &str4)) - to_local_free4 = str4; + &to_local_free4)) + str4 = to_local_free4; else str4 = "(inconvertible)"; } diff --git a/compat/winansi.c b/compat/winansi.c index f83610f684..575813bde8 100644 --- a/compat/winansi.c +++ b/compat/winansi.c @@ -139,7 +139,7 @@ static void write_console(unsigned char *str, size_t len) /* convert utf-8 to utf-16 */ int wlen = xutftowcsn(wbuf, (char*) str, ARRAY_SIZE(wbuf), len); if (wlen < 0) { - wchar_t *err = L"[invalid]"; + const wchar_t *err = L"[invalid]"; WriteConsoleW(console, err, wcslen(err), &dummy, NULL); return; }
Adjust various places in our Win32 compatibility layer where we are not assigning string constants to `const char *` variables. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- compat/basename.c | 18 ++++++++++++++++-- compat/mingw.c | 28 ++++++++++++++++------------ compat/winansi.c | 2 +- 3 files changed, 33 insertions(+), 15 deletions(-)