From 5e0be134d35a31f41921f89331a95337bb38c152 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 5 Feb 2015 01:53:28 -0500 Subject: [PATCH 1/2] config: do not ungetc EOF When we are parsing a config value, if we see a carriage return, we fgetc the next character to see if it is a line feed (in which case we silently drop the CR). If it isn't, we then ungetc the character, and take the literal CR. But we never check whether we in fact got a character at all. If the config file ends in CR, we will get EOF here, and try to ungetc EOF. This works OK for a real stdio stream. The ungetc returns an error, and the next fgetc will then return EOF again. However, our custom buffer-based stream is not so fortunate. It happily rewinds the position of the stream by one character, ignoring the fact that we fed it EOF. The next fgetc call returns the final CR again, over and over, and we end up in an infinite loop. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- config.c | 3 ++- t/t1307-config-blob.sh | 9 +++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/config.c b/config.c index 61a9c296f4..5eca17b242 100644 --- a/config.c +++ b/config.c @@ -228,7 +228,8 @@ static int get_next_char(void) /* DOS like systems */ c = cf->do_fgetc(cf); if (c != '\n') { - cf->do_ungetc(c, cf); + if (c != EOF) + cf->do_ungetc(c, cf); c = '\r'; } } diff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh index fdc257e66f..3c6791e6be 100755 --- a/t/t1307-config-blob.sh +++ b/t/t1307-config-blob.sh @@ -67,4 +67,13 @@ test_expect_success 'parse errors in blobs are properly attributed' ' grep "HEAD:config" err ' +test_expect_success 'can parse blob ending with CR' ' + printf "[some]key = value\\r" >config && + git add config && + git commit -m CR && + echo value >expect && + git config --blob=HEAD:config some.key >actual && + test_cmp expect actual +' + test_done From 1d0655c15ebf7dfb460466d058daab790ed285b2 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 5 Feb 2015 16:00:24 -0500 Subject: [PATCH 2/2] config_buf_ungetc: warn when pushing back a random character Our config code simulates a stdio stream around a buffer, but our fake ungetc() does not behave quite like the real one. In particular, we only rewind the position by one character, but do _not_ actually put the character from the caller into position. It turns out that this does not matter, because we only ever push back the character we just read. In other words, such an assignment would be a noop. But because the function is called ungetc, and because it takes a character parameter, it is a mistake waiting to happen. Actually assigning the character into the buffer would be ideal, but our pointer is actually a "const" copy of the buffer. We do not know who the real owner of the buffer is in this code, and would not want to munge their contents. Instead, we can simply add an assertion that matches what the current caller does, and will let us know if new callers are added that violate the contract. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- config.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/config.c b/config.c index 5eca17b242..788970c6bc 100644 --- a/config.c +++ b/config.c @@ -63,8 +63,12 @@ static int config_buf_fgetc(struct config_source *conf) static int config_buf_ungetc(int c, struct config_source *conf) { - if (conf->u.buf.pos > 0) - return conf->u.buf.buf[--conf->u.buf.pos]; + if (conf->u.buf.pos > 0) { + conf->u.buf.pos--; + if (conf->u.buf.buf[conf->u.buf.pos] != c) + die("BUG: config_buf can only ungetc the same character"); + return c; + } return EOF; }