| Message ID | 82f3908f9620cee29e36a51f6d18ddcc8392b966.1724159575.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Memory leak fixes (pt.5) | expand |
Patrick Steinhardt <ps@pks.im> writes: > When parsing pretty formats from the config we leak the name and user > format whenever these are set multiple times. This is because we do not > free any already-set value in case there is one. > > Plugging this leak for the name is trivial. For the user format we need > to be a bit more careful, because we may end up assigning a pointer into > the allocated region when the string is prefixed with either "format" or > "tformat:". In order to make it safe to unconditionally free the user > format we thus strdup the stripped string into the field instead of a > pointer into the string. Yup, the stripped one is trickier, but the change looks correct. Will queue. By the way, we tend to prefer no spaces after (cast) in our codebase, but I just noticed that it is not spelled out in the coding guidelines. Comparing $ git grep -E -e '\((int|char \*)\) ' \*.c ':!compat/' ':!contrib/' $ git grep -E -e '\((int|char \*)\)[^ ]' \*.c ':!compat/' ':!contrib/' tells me that the extra space after the (cast) is found mostly in borrowed or imported sources and majority of culprits are found in reftable library X-<. Thanks. --- >8 --- Subject: CodingGuidelines: spaces around C operators As we have operated with "write like how your surrounding code is written" for too long, after a huge code drop from another project, we'll end up being inconsistent before such an imported code is cleaned up. We have many uses of cast operator with a space before its operand, mostly in the reftable code. Spell the convention out before it spreads to other places. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Documentation/CodingGuidelines | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git c/Documentation/CodingGuidelines w/Documentation/CodingGuidelines index e4bd0abdcd..ccaea39752 100644 --- c/Documentation/CodingGuidelines +++ w/Documentation/CodingGuidelines @@ -303,7 +303,9 @@ For C programs: v12.01, 2022-03-28). - Variables have to be declared at the beginning of the block, before - the first statement (i.e. -Wdeclaration-after-statement). + the first statement (i.e. -Wdeclaration-after-statement). It is + encouraged to have a blank line between the end of the declarations + and the first statement in the block. - NULL pointers shall be written as NULL, not as 0. @@ -323,6 +325,13 @@ For C programs: while( condition ) func (bar+1); + - A binary operator (other than ",") and ternary conditional "?:" + have a space on each side of the operator to separate it from its + operands. E.g. "A + 1", not "A+1". + + - A unary operator (other than "." and "->") have no space between it + and its operand. E.g. "(char *)ptr", not "(char *) ptr". + - Do not explicitly compare an integral value with constant 0 or '\0', or a pointer value with constant NULL. For instance, to validate that counted array <ptr, cnt> is initialized but has no elements, write:
On Tue, Aug 20, 2024 at 01:36:11PM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > When parsing pretty formats from the config we leak the name and user > > format whenever these are set multiple times. This is because we do not > > free any already-set value in case there is one. > > > > Plugging this leak for the name is trivial. For the user format we need > > to be a bit more careful, because we may end up assigning a pointer into > > the allocated region when the string is prefixed with either "format" or > > "tformat:". In order to make it safe to unconditionally free the user > > format we thus strdup the stripped string into the field instead of a > > pointer into the string. > > Yup, the stripped one is trickier, but the change looks correct. > > Will queue. > > By the way, we tend to prefer no spaces after (cast) in our > codebase, but I just noticed that it is not spelled out in the > coding guidelines. Comparing > > $ git grep -E -e '\((int|char \*)\) ' \*.c ':!compat/' ':!contrib/' > $ git grep -E -e '\((int|char \*)\)[^ ]' \*.c ':!compat/' ':!contrib/' > > tells me that the extra space after the (cast) is found mostly in > borrowed or imported sources and majority of culprits are found in > reftable library X-<. Not entirely surprising. I myself have typically favored adding a space after the cast, and I remember wondering about the coding style several times here. I never wondered enough to actually check though. So thanks for clarifying and thanks for updating the coding guidelines around this. Patrick
diff --git a/pretty.c b/pretty.c index 44222fb83c6..af8f433cdcb 100644 --- a/pretty.c +++ b/pretty.c @@ -63,7 +63,7 @@ static int git_pretty_formats_config(const char *var, const char *value, void *cb UNUSED) { struct cmt_fmt_map *commit_format = NULL; - const char *name; + const char *name, *stripped; char *fmt; int i; @@ -90,15 +90,21 @@ static int git_pretty_formats_config(const char *var, const char *value, commit_formats_len++; } + free((char *) commit_format->name); commit_format->name = xstrdup(name); commit_format->format = CMIT_FMT_USERFORMAT; if (git_config_string(&fmt, var, value)) return -1; - if (skip_prefix(fmt, "format:", &commit_format->user_format)) { + free((char *) commit_format->user_format); + if (skip_prefix(fmt, "format:", &stripped)) { commit_format->is_tformat = 0; - } else if (skip_prefix(fmt, "tformat:", &commit_format->user_format)) { + commit_format->user_format = xstrdup(stripped); + free(fmt); + } else if (skip_prefix(fmt, "tformat:", &stripped)) { commit_format->is_tformat = 1; + commit_format->user_format = xstrdup(stripped); + free(fmt); } else if (strchr(fmt, '%')) { commit_format->is_tformat = 1; commit_format->user_format = fmt;
When parsing pretty formats from the config we leak the name and user format whenever these are set multiple times. This is because we do not free any already-set value in case there is one. Plugging this leak for the name is trivial. For the user format we need to be a bit more careful, because we may end up assigning a pointer into the allocated region when the string is prefixed with either "format" or "tformat:". In order to make it safe to unconditionally free the user format we thus strdup the stripped string into the field instead of a pointer into the string. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- pretty.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)