# [PATCH 7/6] t5401: Use a bare repository for the remote peer 10 messages from 2010-02-10 to 2010-02-11. Participants: Shawn O. Pearce, Johannes Sixt, Junio C Hamano. Thread: https://gitlist.dev/t/22594 ## Shawn O. Pearce, 2010-02-10 02:01 Subject: [PATCH 7/6] t5401: Use a bare repository for the remote peer Message-ID: <1265767290-25863-1-git-send-email-spearce@spearce.org> URL: https://gitlist.dev/e/1265767290-25863-1-git-send-email-spearce%40spearce.org ``` We want to avoid the warnings (or later, test failures) about updating the current branch. It was never my intention to have this test deal with a repository with a working directory, and it is a very old bug that the test even used a non-bare repository for the remote side of the push operations. This fixes the interleaved output error we were seeing as a test failure by avoiding the giant warning message we were getting back about updating the current branch being risky. Its not a real fix, but is something we should do no matter what, because the behavior will change in the future to reject, and the test would break at that time. Signed-off-by: Shawn O. Pearce --- t/t5401-update-hooks.sh | 58 +++++++++++++++++++++++----------------------- 1 files changed, 29 insertions(+), 29 deletions(-) diff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh index c3cf397..7240fab 100755 --- a/t/t5401-update-hooks.sh +++ b/t/t5401-update-hooks.sh @@ -17,22 +17,22 @@ test_expect_success setup ' commit1=$(echo modify | git commit-tree $tree1 -p $commit0) && git update-ref refs/heads/master $commit0 && git update-ref refs/heads/tofail $commit1 && - git clone ./. victim && - GIT_DIR=victim/.git git update-ref refs/heads/tofail $commit1 && + git clone --bare ./. victim.git && + GIT_DIR=victim.git git update-ref refs/heads/tofail $commit1 && git update-ref refs/heads/master $commit1 && git update-ref refs/heads/tofail $commit0 ' -cat >victim/.git/hooks/pre-receive <<'EOF' +cat >victim.git/hooks/pre-receive <<'EOF' #!/bin/sh printf %s "$@" >>$GIT_DIR/pre-receive.args cat - >$GIT_DIR/pre-receive.stdin echo STDOUT pre-receive echo STDERR pre-receive >&2 EOF -chmod u+x victim/.git/hooks/pre-receive +chmod u+x victim.git/hooks/pre-receive -cat >victim/.git/hooks/update <<'EOF' +cat >victim.git/hooks/update <<'EOF' #!/bin/sh echo "$@" >>$GIT_DIR/update.args read x; printf %s "$x" >$GIT_DIR/update.stdin @@ -40,77 +40,77 @@ echo STDOUT update $1 echo STDERR update $1 >&2 test "$1" = refs/heads/master || exit EOF -chmod u+x victim/.git/hooks/update +chmod u+x victim.git/hooks/update -cat >victim/.git/hooks/post-receive <<'EOF' +cat >victim.git/hooks/post-receive <<'EOF' #!/bin/sh printf %s "$@" >>$GIT_DIR/post-receive.args cat - >$GIT_DIR/post-receive.stdin echo STDOUT post-receive echo STDERR post-receive >&2 EOF -chmod u+x victim/.git/hooks/post-receive +chmod u+x victim.git/hooks/post-receive -cat >victim/.git/hooks/post-update <<'EOF' +cat >victim.git/hooks/post-update <<'EOF' #!/bin/sh echo "$@" >>$GIT_DIR/post-update.args read x; printf %s "$x" >$GIT_DIR/post-update.stdin echo STDOUT post-update echo STDERR post-update >&2 EOF -chmod u+x victim/.git/hooks/post-update +chmod u+x victim.git/hooks/post-update test_expect_success push ' - test_must_fail git send-pack --force ./victim/.git \ + test_must_fail git send-pack --force ./victim.git \ master tofail >send.out 2>send.err ' test_expect_success 'updated as expected' ' - test $(GIT_DIR=victim/.git git rev-parse master) = $commit1 && - test $(GIT_DIR=victim/.git git rev-parse tofail) = $commit1 + test $(GIT_DIR=victim.git git rev-parse master) = $commit1 && + test $(GIT_DIR=victim.git git rev-parse tofail) = $commit1 ' test_expect_success 'hooks ran' ' - test -f victim/.git/pre-receive.args && - test -f victim/.git/pre-receive.stdin && - test -f victim/.git/update.args && - test -f victim/.git/update.stdin && - test -f victim/.git/post-receive.args && - test -f victim/.git/post-receive.stdin && - test -f victim/.git/post-update.args && - test -f victim/.git/post-update.stdin + test -f victim.git/pre-receive.args && + test -f victim.git/pre-receive.stdin && + test -f victim.git/update.args && + test -f victim.git/update.stdin && + test -f victim.git/post-receive.args && + test -f victim.git/post-receive.stdin && + test -f victim.git/post-update.args && + test -f victim.git/post-update.stdin ' test_expect_success 'pre-receive hook input' ' (echo $commit0 $commit1 refs/heads/master; echo $commit1 $commit0 refs/heads/tofail - ) | test_cmp - victim/.git/pre-receive.stdin + ) | test_cmp - victim.git/pre-receive.stdin ' test_expect_success 'update hook arguments' ' (echo refs/heads/master $commit0 $commit1; echo refs/heads/tofail $commit1 $commit0 - ) | test_cmp - victim/.git/update.args + ) | test_cmp - victim.git/update.args ' test_expect_success 'post-receive hook input' ' echo $commit0 $commit1 refs/heads/master | - test_cmp - victim/.git/post-receive.stdin + test_cmp - victim.git/post-receive.stdin ' test_expect_success 'post-update hook arguments' ' echo refs/heads/master | - test_cmp - victim/.git/post-update.args + test_cmp - victim.git/post-update.args ' test_expect_success 'all hook stdin is /dev/null' ' - ! test -s victim/.git/update.stdin && - ! test -s victim/.git/post-update.stdin + ! test -s victim.git/update.stdin && + ! test -s victim.git/post-update.stdin ' test_expect_success 'all *-receive hook args are empty' ' - ! test -s victim/.git/pre-receive.args && - ! test -s victim/.git/post-receive.args + ! test -s victim.git/pre-receive.args && + ! test -s victim.git/post-receive.args ' test_expect_success 'send-pack produced no output' ' -- 1.7.0.rc2.170.gbc565 ``` ## Shawn O. Pearce, 2010-02-10 02:01 Subject: [PATCH 8/6] receive-pack: Send internal errors over side-band #2 Message-ID: <1265767290-25863-2-git-send-email-spearce@spearce.org> URL: https://gitlist.dev/e/1265767290-25863-2-git-send-email-spearce%40spearce.org In-Reply-To: <1265767290-25863-1-git-send-email-spearce@spearce.org> ``` If the client has requested side-band-64k capability, send any of the internal error or warning messages in the muxed side-band stream using the same band as our hook output, band #2. By putting everything in one stream we ensure all messages are processed by the side-band demuxer, avoiding interleaving between our own stderr and the side-band demuxer's stderr buffers. Signed-off-by: Shawn O. Pearce --- builtin-receive-pack.c | 68 +++++++++++++++++++++++++++++++++++----------- t/t5401-update-hooks.sh | 3 +- 2 files changed, 53 insertions(+), 18 deletions(-) diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c index da1c26b..e98c2f1 100644 --- a/builtin-receive-pack.c +++ b/builtin-receive-pack.c @@ -139,6 +139,40 @@ static struct command *commands; static const char pre_receive_hook[] = "hooks/pre-receive"; static const char post_receive_hook[] = "hooks/post-receive"; +static void rp_error(const char *err, ...) __attribute__((format (printf, 1, 2))); +static void rp_warning(const char *err, ...) __attribute__((format (printf, 1, 2))); + +static void report_message(const char *prefix, const char *err, va_list params) +{ + int sz = strlen(prefix); + char msg[4096]; + + strncpy(msg, prefix, sz); + sz += vsnprintf(msg + sz, sizeof(msg) - (sz + 1), err, params); + msg[sz++] = '\n'; + + if (use_sideband) + send_sideband(1, 2, msg, sz, use_sideband); + else + xwrite(2, msg, sz); +} + +static void rp_warning(const char *err, ...) +{ + va_list params; + va_start(params, err); + report_message("warning: ", err, params); + va_end(params); +} + +static void rp_error(const char *err, ...) +{ + va_list params; + va_start(params, err); + report_message("error: ", err, params); + va_end(params); +} + static int copy_to_sideband(int in, int out, void *arg) { char data[128]; @@ -276,7 +310,7 @@ static void warn_unconfigured_deny(void) { int i; for (i = 0; i < ARRAY_SIZE(warn_unconfigured_deny_msg); i++) - warning("%s", warn_unconfigured_deny_msg[i]); + rp_warning("%s", warn_unconfigured_deny_msg[i]); } static char *warn_unconfigured_deny_delete_current_msg[] = { @@ -302,7 +336,7 @@ static void warn_unconfigured_deny_delete_current(void) for (i = 0; i < ARRAY_SIZE(warn_unconfigured_deny_delete_current_msg); i++) - warning("%s", warn_unconfigured_deny_delete_current_msg[i]); + rp_warning("%s", warn_unconfigured_deny_delete_current_msg[i]); } static const char *update(struct command *cmd) @@ -314,7 +348,7 @@ static const char *update(struct command *cmd) /* only refs/... are allowed */ if (prefixcmp(name, "refs/") || check_ref_format(name + 5)) { - error("refusing to create funny ref '%s' remotely", name); + rp_error("refusing to create funny ref '%s' remotely", name); return "funny refname"; } @@ -324,25 +358,25 @@ static const char *update(struct command *cmd) break; case DENY_UNCONFIGURED: case DENY_WARN: - warning("updating the current branch"); + rp_warning("updating the current branch"); if (deny_current_branch == DENY_UNCONFIGURED) warn_unconfigured_deny(); break; case DENY_REFUSE: - error("refusing to update checked out branch: %s", name); + rp_error("refusing to update checked out branch: %s", name); return "branch is currently checked out"; } } if (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) { - error("unpack should have generated %s, " - "but I can't find it!", sha1_to_hex(new_sha1)); + rp_error("unpack should have generated %s, " + "but I can't find it!", sha1_to_hex(new_sha1)); return "bad pack"; } if (!is_null_sha1(old_sha1) && is_null_sha1(new_sha1)) { if (deny_deletes && !prefixcmp(name, "refs/heads/")) { - error("denying ref deletion for %s", name); + rp_error("denying ref deletion for %s", name); return "deletion prohibited"; } @@ -354,10 +388,10 @@ static const char *update(struct command *cmd) case DENY_UNCONFIGURED: if (deny_delete_current == DENY_UNCONFIGURED) warn_unconfigured_deny_delete_current(); - warning("deleting the current branch"); + rp_warning("deleting the current branch"); break; case DENY_REFUSE: - error("refusing to delete the current branch: %s", name); + rp_error("refusing to delete the current branch: %s", name); return "deletion of the current branch prohibited"; } } @@ -376,7 +410,7 @@ static const char *update(struct command *cmd) if (!old_object || !new_object || old_object->type != OBJ_COMMIT || new_object->type != OBJ_COMMIT) { - error("bad sha1 objects for %s", name); + rp_error("bad sha1 objects for %s", name); return "bad ref"; } old_commit = (struct commit *)old_object; @@ -387,23 +421,23 @@ static const char *update(struct command *cmd) break; free_commit_list(bases); if (!ent) { - error("denying non-fast-forward %s" - " (you should pull first)", name); + rp_error("denying non-fast-forward %s" + " (you should pull first)", name); return "non-fast-forward"; } } if (run_update_hook(cmd)) { - error("hook declined to update %s", name); + rp_error("hook declined to update %s", name); return "hook declined"; } if (is_null_sha1(new_sha1)) { if (!parse_object(old_sha1)) { - warning ("Allowing deletion of corrupt ref."); + rp_warning("Allowing deletion of corrupt ref."); old_sha1 = NULL; } if (delete_ref(name, old_sha1, 0)) { - error("failed to delete %s", name); + rp_error("failed to delete %s", name); return "failed to delete"; } return NULL; /* good */ @@ -411,7 +445,7 @@ static const char *update(struct command *cmd) else { lock = lock_any_ref_for_update(name, old_sha1, 0); if (!lock) { - error("failed to lock %s", name); + rp_error("failed to lock %s", name); return "failed to lock"; } if (write_ref_sha1(lock, new_sha1, "push")) { diff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh index 7240fab..17bcb0b 100755 --- a/t/t5401-update-hooks.sh +++ b/t/t5401-update-hooks.sh @@ -124,6 +124,7 @@ remote: STDOUT update refs/heads/master remote: STDERR update refs/heads/master remote: STDOUT update refs/heads/tofail remote: STDERR update refs/heads/tofail +remote: error: hook declined to update refs/heads/tofail remote: STDOUT post-receive remote: STDERR post-receive remote: STDOUT post-update @@ -131,7 +132,7 @@ remote: STDERR post-update EOF test_expect_success 'send-pack stderr contains hook messages' ' grep ^remote: send.err | sed "s/ *\$//" >actual && - test_cmp - actual URL: https://gitlist.dev/e/4B725CB1.1080908%40viscovery.net In-Reply-To: <1265767290-25863-2-git-send-email-spearce@spearce.org> ``` Shawn O. Pearce schrieb: > +static void rp_error(const char *err, ...) __attribute__((format (printf, 1, 2))); > +static void rp_warning(const char *err, ...) __attribute__((format (printf, 1, 2))); > + > +static void report_message(const char *prefix, const char *err, va_list params) > +{ > + int sz = strlen(prefix); > + char msg[4096]; > + > + strncpy(msg, prefix, sz); > + sz += vsnprintf(msg + sz, sizeof(msg) - (sz + 1), err, params); > + msg[sz++] = '\n'; This writes beyond the buffer if it is too small because the return value tells how many characters *would* have been written if it were sufficiently large, no? > + > + if (use_sideband) > + send_sideband(1, 2, msg, sz, use_sideband); > + else > + xwrite(2, msg, sz); > +} > + > +static void rp_warning(const char *err, ...) > ... > +static void rp_error(const char *err, ...) > ... Looks like we need set_report_routine(). Or did you replace only selected error() and warning() calls by rp_error() and rp_warning()? -- Hannes ``` ## Junio C Hamano, 2010-02-10 07:23 Subject: Re: [PATCH 8/6] receive-pack: Send internal errors over side-band #2 Message-ID: <7vwryleeq5.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vwryleeq5.fsf%40alter.siamese.dyndns.org In-Reply-To: <4B725CB1.1080908@viscovery.net> ``` Johannes Sixt writes: > Looks like we need set_report_routine(). > > Or did you replace only selected error() and warning() calls by rp_error() > and rp_warning()? That actually sounds like a good suggestion. ``` ## Johannes Sixt, 2010-02-10 08:13 Subject: Re: [PATCH 8/6] receive-pack: Send internal errors over side-band #2 Message-ID: <4B726A8C.6010600@viscovery.net> URL: https://gitlist.dev/e/4B726A8C.6010600%40viscovery.net In-Reply-To: <4B725CB1.1080908@viscovery.net> ``` Johannes Sixt schrieb: > Or did you replace only selected error() and warning() calls by rp_error() > and rp_warning()? Actually, you want to send only selected messages to the pusher. For example, these look like errors on the server side (right?) and should go to the site administrator: > if (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) { > - error("unpack should have generated %s, " > - "but I can't find it!", sha1_to_hex(new_sha1)); > + rp_error("unpack should have generated %s, " > + "but I can't find it!", sha1_to_hex(new_sha1)); > return "bad pack"; > } > if (!old_object || !new_object || > old_object->type != OBJ_COMMIT || > new_object->type != OBJ_COMMIT) { > - error("bad sha1 objects for %s", name); > + rp_error("bad sha1 objects for %s", name); > return "bad ref"; > } > old_commit = (struct commit *)old_object; In particular, your patch does not send errors produced by unpack-objects or index-pack to the pusher over the sideband, and this is the right thing to do. -- Hannes ``` ## Shawn O. Pearce, 2010-02-10 17:17 Subject: Re: [PATCH 8/6] receive-pack: Send internal errors over side-band #2 Message-ID: <20100210171708.GF2747@spearce.org> URL: https://gitlist.dev/e/20100210171708.GF2747%40spearce.org In-Reply-To: <4B725CB1.1080908@viscovery.net> ``` Johannes Sixt wrote: > Shawn O. Pearce schrieb: > > +static void rp_error(const char *err, ...) __attribute__((format (printf, 1, 2))); > > +static void rp_warning(const char *err, ...) __attribute__((format (printf, 1, 2))); > > + > > +static void report_message(const char *prefix, const char *err, va_list params) > > +{ > > + int sz = strlen(prefix); > > + char msg[4096]; > > + > > + strncpy(msg, prefix, sz); > > + sz += vsnprintf(msg + sz, sizeof(msg) - (sz + 1), err, params); > > + msg[sz++] = '\n'; > > This writes beyond the buffer if it is too small because the return value > tells how many characters *would* have been written if it were > sufficiently large, no? Ugh. I don't code C often enough anymore. Thank you for catching that. > > +static void rp_warning(const char *err, ...) > > ... > > +static void rp_error(const char *err, ...) > > ... > > Looks like we need set_report_routine(). > > Or did you replace only selected error() and warning() calls by rp_error() > and rp_warning()? As described by others in the thread... I only replaced selected calls. Well, most of them, maybe too many. But I didn't want to expose everything to the client. So I added new functions. Yes, it was painful. I wasn't happy about it. But I also wasn't happy about exposing every message to the client over the side-band channel. -- Shawn. ``` ## Shawn O. Pearce, 2010-02-10 17:34 Subject: [PATCH 8/6 v2] receive-pack: Send internal errors over side-band #2 Message-ID: <20100210173412.GG2747@spearce.org> URL: https://gitlist.dev/e/20100210173412.GG2747%40spearce.org In-Reply-To: <4B726A8C.6010600@viscovery.net> ``` If the client has requested side-band-64k capability, send any of the internal error or warning messages in the muxed side-band stream using the same band as our hook output, band #2. By putting everything in one stream we ensure all messages are processed by the side-band demuxer, avoiding interleaving between our own stderr and the side-band demuxer's stderr buffers. Signed-off-by: Shawn O. Pearce --- - Buffer overflow fixed. - Kept the two corruption errors on the server side. builtin-receive-pack.c | 64 ++++++++++++++++++++++++++++++++++++---------- t/t5401-update-hooks.sh | 3 +- 2 files changed, 52 insertions(+), 15 deletions(-) diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c index da1c26b..a5543f9 100644 --- a/builtin-receive-pack.c +++ b/builtin-receive-pack.c @@ -139,6 +139,42 @@ static struct command *commands; static const char pre_receive_hook[] = "hooks/pre-receive"; static const char post_receive_hook[] = "hooks/post-receive"; +static void rp_error(const char *err, ...) __attribute__((format (printf, 1, 2))); +static void rp_warning(const char *err, ...) __attribute__((format (printf, 1, 2))); + +static void report_message(const char *prefix, const char *err, va_list params) +{ + int sz = strlen(prefix); + char msg[4096]; + + strncpy(msg, prefix, sz); + sz += vsnprintf(msg + sz, sizeof(msg) - sz, err, params); + if (sz > (sizeof(msg) - 1)) + sz = sizeof(msg) - 1; + msg[sz++] = '\n'; + + if (use_sideband) + send_sideband(1, 2, msg, sz, use_sideband); + else + xwrite(2, msg, sz); +} + +static void rp_warning(const char *err, ...) +{ + va_list params; + va_start(params, err); + report_message("warning: ", err, params); + va_end(params); +} + +static void rp_error(const char *err, ...) +{ + va_list params; + va_start(params, err); + report_message("error: ", err, params); + va_end(params); +} + static int copy_to_sideband(int in, int out, void *arg) { char data[128]; @@ -276,7 +312,7 @@ static void warn_unconfigured_deny(void) { int i; for (i = 0; i < ARRAY_SIZE(warn_unconfigured_deny_msg); i++) - warning("%s", warn_unconfigured_deny_msg[i]); + rp_warning("%s", warn_unconfigured_deny_msg[i]); } static char *warn_unconfigured_deny_delete_current_msg[] = { @@ -302,7 +338,7 @@ static void warn_unconfigured_deny_delete_current(void) for (i = 0; i < ARRAY_SIZE(warn_unconfigured_deny_delete_current_msg); i++) - warning("%s", warn_unconfigured_deny_delete_current_msg[i]); + rp_warning("%s", warn_unconfigured_deny_delete_current_msg[i]); } static const char *update(struct command *cmd) @@ -314,7 +350,7 @@ static const char *update(struct command *cmd) /* only refs/... are allowed */ if (prefixcmp(name, "refs/") || check_ref_format(name + 5)) { - error("refusing to create funny ref '%s' remotely", name); + rp_error("refusing to create funny ref '%s' remotely", name); return "funny refname"; } @@ -324,12 +360,12 @@ static const char *update(struct command *cmd) break; case DENY_UNCONFIGURED: case DENY_WARN: - warning("updating the current branch"); + rp_warning("updating the current branch"); if (deny_current_branch == DENY_UNCONFIGURED) warn_unconfigured_deny(); break; case DENY_REFUSE: - error("refusing to update checked out branch: %s", name); + rp_error("refusing to update checked out branch: %s", name); return "branch is currently checked out"; } } @@ -342,7 +378,7 @@ static const char *update(struct command *cmd) if (!is_null_sha1(old_sha1) && is_null_sha1(new_sha1)) { if (deny_deletes && !prefixcmp(name, "refs/heads/")) { - error("denying ref deletion for %s", name); + rp_error("denying ref deletion for %s", name); return "deletion prohibited"; } @@ -354,10 +390,10 @@ static const char *update(struct command *cmd) case DENY_UNCONFIGURED: if (deny_delete_current == DENY_UNCONFIGURED) warn_unconfigured_deny_delete_current(); - warning("deleting the current branch"); + rp_warning("deleting the current branch"); break; case DENY_REFUSE: - error("refusing to delete the current branch: %s", name); + rp_error("refusing to delete the current branch: %s", name); return "deletion of the current branch prohibited"; } } @@ -387,23 +423,23 @@ static const char *update(struct command *cmd) break; free_commit_list(bases); if (!ent) { - error("denying non-fast-forward %s" - " (you should pull first)", name); + rp_error("denying non-fast-forward %s" + " (you should pull first)", name); return "non-fast-forward"; } } if (run_update_hook(cmd)) { - error("hook declined to update %s", name); + rp_error("hook declined to update %s", name); return "hook declined"; } if (is_null_sha1(new_sha1)) { if (!parse_object(old_sha1)) { - warning ("Allowing deletion of corrupt ref."); + rp_warning("Allowing deletion of corrupt ref."); old_sha1 = NULL; } if (delete_ref(name, old_sha1, 0)) { - error("failed to delete %s", name); + rp_error("failed to delete %s", name); return "failed to delete"; } return NULL; /* good */ @@ -411,7 +447,7 @@ static const char *update(struct command *cmd) else { lock = lock_any_ref_for_update(name, old_sha1, 0); if (!lock) { - error("failed to lock %s", name); + rp_error("failed to lock %s", name); return "failed to lock"; } if (write_ref_sha1(lock, new_sha1, "push")) { diff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh index 7240fab..17bcb0b 100755 --- a/t/t5401-update-hooks.sh +++ b/t/t5401-update-hooks.sh @@ -124,6 +124,7 @@ remote: STDOUT update refs/heads/master remote: STDERR update refs/heads/master remote: STDOUT update refs/heads/tofail remote: STDERR update refs/heads/tofail +remote: error: hook declined to update refs/heads/tofail remote: STDOUT post-receive remote: STDERR post-receive remote: STDOUT post-update @@ -131,7 +132,7 @@ remote: STDERR post-update EOF test_expect_success 'send-pack stderr contains hook messages' ' grep ^remote: send.err | sed "s/ *\$//" >actual && - test_cmp - actual URL: https://gitlist.dev/e/4B73C0FF.5020503%40viscovery.net In-Reply-To: <20100210173412.GG2747@spearce.org> ``` Shawn O. Pearce schrieb: > +static void report_message(const char *prefix, const char *err, va_list params) > +{ > + int sz = strlen(prefix); > + char msg[4096]; > + > + strncpy(msg, prefix, sz); > + sz += vsnprintf(msg + sz, sizeof(msg) - sz, err, params); > + if (sz > (sizeof(msg) - 1)) > + sz = sizeof(msg) - 1; > + msg[sz++] = '\n'; Sorry, still no joy - the terminating NUL is missing (I should have noticed this in your v1 already). I suggest to forgo the length check for simplicity because this function is only called with data that is already guaranteed to be less than 1000 bytes, i.e.: strncpy(msg, prefix, sz); /* data is guaranteed to fit due to packet length limit in read_head_info() */ sz += vsprintf(msg + sz, err, params); msg[sz++] = '\n'; msg[sz++] = '\0'; -- Hannes ``` ## Shawn O. Pearce, 2010-02-11 15:05 Subject: Re: [PATCH 8/6 v2] receive-pack: Send internal errors over side-band #2 Message-ID: <20100211150547.GL2747@spearce.org> URL: https://gitlist.dev/e/20100211150547.GL2747%40spearce.org In-Reply-To: <4B73C0FF.5020503@viscovery.net> ``` Johannes Sixt wrote: > Shawn O. Pearce schrieb: >> +static void report_message(const char *prefix, const char *err, va_list params) >> +{ >> + int sz = strlen(prefix); >> + char msg[4096]; >> + >> + strncpy(msg, prefix, sz); >> + sz += vsnprintf(msg + sz, sizeof(msg) - sz, err, params); >> + if (sz > (sizeof(msg) - 1)) >> + sz = sizeof(msg) - 1; >> + msg[sz++] = '\n'; > > Sorry, still no joy - the terminating NUL is missing (I should have > noticed this in your v1 already). Why is it necessary? Once the msg buffer is prepared, its written using its length, sz, not its NUL termination status. Neither send_sideband() nor xwrite() care about NUL termination. The only reason to put a NUL onto this buffer is so you can do "p msg" within GDB and get a useful result. We don't typically do this in these contexts. Or did my MTA inject additional C code I didn't write? > I suggest to forgo the length check for > simplicity because this function is only called with data that is already > guaranteed to be less than 1000 bytes, i.e.: > > strncpy(msg, prefix, sz); > /* data is guaranteed to fit due to packet length limit in > read_head_info() */ Yea, that's true now. We probably could have used 1200 bytes for the msg buffer, rather than 4096. Even the huge warnings about current branch behavior are broken up into one string per line. -- Shawn. ``` ## Johannes Sixt, 2010-02-11 19:04 Subject: Re: [PATCH 8/6 v2] receive-pack: Send internal errors over side-band #2 Message-ID: <4B7454B6.50703@viscovery.net> URL: https://gitlist.dev/e/4B7454B6.50703%40viscovery.net In-Reply-To: <20100211150547.GL2747@spearce.org> ``` Shawn O. Pearce schrieb: > Johannes Sixt wrote: >> Sorry, still no joy - the terminating NUL is missing (I should have >> noticed this in your v1 already). > > Why is it necessary? It isn't, sorry - I'm sick, and it shows :-/ Your version is fine. -- Hannes ```