| Message ID | ff0db7e3-abce-44ea-a1e3-16e1fdaf4c75@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] receive-pack: use find_commit_header() in check_cert_push_options() | expand |
René Scharfe <l.s.r@web.de> writes: > The string comparison becomes more complicated because we need to check > for NUL explicitly after comparing the length-limited option, but on the > flip side we don't need to clean up allocations or track the remaining > buffer length. Yeah, the strncmp() followed by the termination check indeed is trickier but not having to worry about allocation is nice. > if (options_seen > push_options->nr > - || strcmp(option, > - push_options->items[options_seen - 1].string)) { We used to allocate option[] with NUL termination, but ... > - retval = 0; > - goto leave; > - } > - free(option); > + || strncmp(push_options->items[options_seen - 1].string, > + option, optionlen) > + || push_options->items[options_seen - 1].string[optionlen]) ... now option[] is a borrowed memory, option[optionlen] would have been NUL if we were allocating. So to see if the last-seen string[] is different from option[], we have to see that they match up to optionlen and the last-seen string[] ends there. Trickier than before, but is correct. > + return 0; > } > > if (options_seen != push_options->nr) > retval = 0; > > -leave: > - free(option); > return retval; > } > > -- > 2.43.0
Am 09.02.24 um 23:11 schrieb Junio C Hamano: > René Scharfe <l.s.r@web.de> writes: > >> The string comparison becomes more complicated because we need to check >> for NUL explicitly after comparing the length-limited option, but on the >> flip side we don't need to clean up allocations or track the remaining >> buffer length. > > Yeah, the strncmp() followed by the termination check indeed is > trickier but not having to worry about allocation is nice. > >> if (options_seen > push_options->nr >> - || strcmp(option, >> - push_options->items[options_seen - 1].string)) { > > We used to allocate option[] with NUL termination, but ... > >> - retval = 0; >> - goto leave; >> - } >> - free(option); >> + || strncmp(push_options->items[options_seen - 1].string, >> + option, optionlen) >> + || push_options->items[options_seen - 1].string[optionlen]) > > ... now option[] is a borrowed memory, option[optionlen] would have > been NUL if we were allocating. So to see if the last-seen string[] > is different from option[], we have to see that they match up to > optionlen and the last-seen string[] ends there. Trickier than > before, but is correct. I just discovered 14570dc67d (wrapper: add function to compare strings with different NUL termination, 2020-05-25). Perhaps squash this in to simplify? --- builtin/receive-pack.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index dbee508775..db65607485 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -717,9 +717,8 @@ static int check_cert_push_options(const struct string_list *push_options) buf = option + optionlen + 1; options_seen++; if (options_seen > push_options->nr - || strncmp(push_options->items[options_seen - 1].string, - option, optionlen) - || push_options->items[options_seen - 1].string[optionlen]) + || xstrncmpz(push_options->items[options_seen - 1].string, + option, optionlen)) return 0; } -- 2.43.0
René Scharfe <l.s.r@web.de> writes: > I just discovered 14570dc67d (wrapper: add function to compare strings > with different NUL termination, 2020-05-25). Perhaps squash this in to > simplify? Oooh, xstrncmpz() seems to target this exact use case. Nice. > > --- > builtin/receive-pack.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c > index dbee508775..db65607485 100644 > --- a/builtin/receive-pack.c > +++ b/builtin/receive-pack.c > @@ -717,9 +717,8 @@ static int check_cert_push_options(const struct string_list *push_options) > buf = option + optionlen + 1; > options_seen++; > if (options_seen > push_options->nr > - || strncmp(push_options->items[options_seen - 1].string, > - option, optionlen) > - || push_options->items[options_seen - 1].string[optionlen]) > + || xstrncmpz(push_options->items[options_seen - 1].string, > + option, optionlen)) > return 0; > } > > -- > 2.43.0
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index e36b1d619f..8ebb3a872f 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -718,35 +718,29 @@ static const char *check_nonce(const char *buf, size_t len) static int check_cert_push_options(const struct string_list *push_options) { const char *buf = push_cert.buf; - int len = push_cert.len; - char *option; - const char *next_line; + const char *option; + size_t optionlen; int options_seen = 0; int retval = 1; - if (!len) + if (!*buf) return 1; - while ((option = find_header(buf, len, "push-option", &next_line))) { - len -= (next_line - buf); - buf = next_line; + while ((option = find_commit_header(buf, "push-option", &optionlen))) { + buf = option + optionlen + 1; options_seen++; if (options_seen > push_options->nr - || strcmp(option, - push_options->items[options_seen - 1].string)) { - retval = 0; - goto leave; - } - free(option); + || strncmp(push_options->items[options_seen - 1].string, + option, optionlen) + || push_options->items[options_seen - 1].string[optionlen]) + return 0; } if (options_seen != push_options->nr) retval = 0; -leave: - free(option); return retval; }
Use the public function find_commit_header() instead of find_header() to simplify the code. This is possible and safe because we're operating on a strbuf, which is always NUL-terminated, so there is no risk of running over the end of the buffer. It cannot contain NUL within the buffer, as it is built using strbuf_addstr(), only. The string comparison becomes more complicated because we need to check for NUL explicitly after comparing the length-limited option, but on the flip side we don't need to clean up allocations or track the remaining buffer length. Signed-off-by: René Scharfe <l.s.r@web.de> --- builtin/receive-pack.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) -- 2.43.0