* [PATCH 7/6] t5401: Use a bare repository for the remote peer @ 2010-02-10 2:01 Shawn O. Pearce 2010-02-10 2:01 ` [PATCH 8/6] receive-pack: Send internal errors over side-band #2 Shawn O. Pearce 0 siblings, 1 reply; 10+ messages in thread From: Shawn O. Pearce @ 2010-02-10 2:01 UTC (permalink / raw) To: git 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 <spearce@spearce.org> --- 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 8/6] receive-pack: Send internal errors over side-band #2 2010-02-10 2:01 [PATCH 7/6] t5401: Use a bare repository for the remote peer Shawn O. Pearce @ 2010-02-10 2:01 ` Shawn O. Pearce 2010-02-10 7:13 ` Johannes Sixt 0 siblings, 1 reply; 10+ messages in thread From: Shawn O. Pearce @ 2010-02-10 2:01 UTC (permalink / raw) To: git 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 <spearce@spearce.org> --- 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 <expect + test_cmp expect actual ' test_done -- 1.7.0.rc2.170.gbc565 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 8/6] receive-pack: Send internal errors over side-band #2 2010-02-10 2:01 ` [PATCH 8/6] receive-pack: Send internal errors over side-band #2 Shawn O. Pearce @ 2010-02-10 7:13 ` Johannes Sixt 2010-02-10 7:23 ` Junio C Hamano ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Johannes Sixt @ 2010-02-10 7:13 UTC (permalink / raw) To: Shawn O. Pearce; +Cc: git 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 8/6] receive-pack: Send internal errors over side-band #2 2010-02-10 7:13 ` Johannes Sixt @ 2010-02-10 7:23 ` Junio C Hamano 2010-02-10 8:13 ` Johannes Sixt 2010-02-10 17:17 ` [PATCH 8/6] " Shawn O. Pearce 2 siblings, 0 replies; 10+ messages in thread From: Junio C Hamano @ 2010-02-10 7:23 UTC (permalink / raw) To: Johannes Sixt; +Cc: Shawn O. Pearce, git Johannes Sixt <j.sixt@viscovery.net> 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 8/6] receive-pack: Send internal errors over side-band #2 2010-02-10 7:13 ` Johannes Sixt 2010-02-10 7:23 ` Junio C Hamano @ 2010-02-10 8:13 ` Johannes Sixt 2010-02-10 17:34 ` [PATCH 8/6 v2] " Shawn O. Pearce 2010-02-10 17:17 ` [PATCH 8/6] " Shawn O. Pearce 2 siblings, 1 reply; 10+ messages in thread From: Johannes Sixt @ 2010-02-10 8:13 UTC (permalink / raw) To: Shawn O. Pearce; +Cc: git 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 8/6 v2] receive-pack: Send internal errors over side-band #2 2010-02-10 8:13 ` Johannes Sixt @ 2010-02-10 17:34 ` Shawn O. Pearce 2010-02-11 8:34 ` Johannes Sixt 0 siblings, 1 reply; 10+ messages in thread From: Shawn O. Pearce @ 2010-02-10 17:34 UTC (permalink / raw) To: Johannes Sixt, Junio C Hamano; +Cc: git 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 <spearce@spearce.org> --- - 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 <expect + test_cmp expect actual ' test_done -- 1.7.0.rc2.170.gbc565 -- Shawn. ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 8/6 v2] receive-pack: Send internal errors over side-band #2 2010-02-10 17:34 ` [PATCH 8/6 v2] " Shawn O. Pearce @ 2010-02-11 8:34 ` Johannes Sixt 2010-02-11 15:05 ` Shawn O. Pearce 0 siblings, 1 reply; 10+ messages in thread From: Johannes Sixt @ 2010-02-11 8:34 UTC (permalink / raw) To: Shawn O. Pearce; +Cc: Junio C Hamano, git 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 8/6 v2] receive-pack: Send internal errors over side-band #2 2010-02-11 8:34 ` Johannes Sixt @ 2010-02-11 15:05 ` Shawn O. Pearce 2010-02-11 19:04 ` Johannes Sixt 0 siblings, 1 reply; 10+ messages in thread From: Shawn O. Pearce @ 2010-02-11 15:05 UTC (permalink / raw) To: Johannes Sixt; +Cc: Junio C Hamano, git Johannes Sixt <j.sixt@viscovery.net> 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 8/6 v2] receive-pack: Send internal errors over side-band #2 2010-02-11 15:05 ` Shawn O. Pearce @ 2010-02-11 19:04 ` Johannes Sixt 0 siblings, 0 replies; 10+ messages in thread From: Johannes Sixt @ 2010-02-11 19:04 UTC (permalink / raw) To: Shawn O. Pearce; +Cc: Junio C Hamano, git Shawn O. Pearce schrieb: > Johannes Sixt <j.sixt@viscovery.net> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 8/6] receive-pack: Send internal errors over side-band #2 2010-02-10 7:13 ` Johannes Sixt 2010-02-10 7:23 ` Junio C Hamano 2010-02-10 8:13 ` Johannes Sixt @ 2010-02-10 17:17 ` Shawn O. Pearce 2 siblings, 0 replies; 10+ messages in thread From: Shawn O. Pearce @ 2010-02-10 17:17 UTC (permalink / raw) To: Johannes Sixt; +Cc: git Johannes Sixt <j.sixt@viscovery.net> 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2010-02-11 19:04 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-02-10 2:01 [PATCH 7/6] t5401: Use a bare repository for the remote peer Shawn O. Pearce 2010-02-10 2:01 ` [PATCH 8/6] receive-pack: Send internal errors over side-band #2 Shawn O. Pearce 2010-02-10 7:13 ` Johannes Sixt 2010-02-10 7:23 ` Junio C Hamano 2010-02-10 8:13 ` Johannes Sixt 2010-02-10 17:34 ` [PATCH 8/6 v2] " Shawn O. Pearce 2010-02-11 8:34 ` Johannes Sixt 2010-02-11 15:05 ` Shawn O. Pearce 2010-02-11 19:04 ` Johannes Sixt 2010-02-10 17:17 ` [PATCH 8/6] " Shawn O. Pearce
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.