{"thread":{"id":"50889","subject":"[PATCH] ls-files: use correct format string","startedAt":"2019-04-07T18:48:01Z","lastAt":"2019-04-11T23:49:24Z","messageCount":4,"participants":["Thomas Gummerer","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"373354","messageId":"20190407184751.28027-1-t.gummerer@gmail.com","threadId":"50889","inReplyTo":null,"subject":"[PATCH] ls-files: use correct format string","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-04-07T18:47:51Z","receivedAt":"2019-04-07T18:48:01Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"struct stat_data and struct cache_time both use unsigned ints for all\ntheir members.  However the format string for 'git ls-files --debug'\ncurrently uses %d for formatting these numbers.  This means that we\npotentially print these values incorrectly if they are greater than\nINT_MAX.\n\nThis has been the case since the --debug option was introduced in 'git\nls-files' in 8497421715 (\"ls-files: learn a debugging dump format\",\n2010-07-31).\n\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n\nI noticed this running 'git ls-files --debug' the other day.\nPresumably there's not too many users of 'git ls-files', and even\nfewer will see values that trigger this behaviour.  \n\n builtin/ls-files.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 29a8762d46..463105ccb5 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -112,11 +112,11 @@ static void print_debug(const struct cache_entry *ce)\n \tif (debug_mode) {\n \t\tconst struct stat_data *sd = &ce->ce_stat_data;\n \n-\t\tprintf(\"  ctime: %d:%d\\n\", sd->sd_ctime.sec, sd->sd_ctime.nsec);\n-\t\tprintf(\"  mtime: %d:%d\\n\", sd->sd_mtime.sec, sd->sd_mtime.nsec);\n-\t\tprintf(\"  dev: %d\\tino: %d\\n\", sd->sd_dev, sd->sd_ino);\n-\t\tprintf(\"  uid: %d\\tgid: %d\\n\", sd->sd_uid, sd->sd_gid);\n-\t\tprintf(\"  size: %d\\tflags: %x\\n\", sd->sd_size, ce->ce_flags);\n+\t\tprintf(\"  ctime: %u:%u\\n\", sd->sd_ctime.sec, sd->sd_ctime.nsec);\n+\t\tprintf(\"  mtime: %u:%u\\n\", sd->sd_mtime.sec, sd->sd_mtime.nsec);\n+\t\tprintf(\"  dev: %u\\tino: %u\\n\", sd->sd_dev, sd->sd_ino);\n+\t\tprintf(\"  uid: %u\\tgid: %u\\n\", sd->sd_uid, sd->sd_gid);\n+\t\tprintf(\"  size: %u\\tflags: %x\\n\", sd->sd_size, ce->ce_flags);\n \t}\n }\n \n-- \n2.21.0.392.gf8f6787159\n\n"},{"id":"373626","messageId":"20190411041823.GA17699@sigill.intra.peff.net","threadId":"50889","inReplyTo":"20190407184751.28027-1-t.gummerer@gmail.com","subject":"Re: [PATCH] ls-files: use correct format string","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-11T04:18:24Z","receivedAt":"2019-04-11T04:18:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 07, 2019 at 07:47:51PM +0100, Thomas Gummerer wrote:\n\n> struct stat_data and struct cache_time both use unsigned ints for all\n> their members.  However the format string for 'git ls-files --debug'\n> currently uses %d for formatting these numbers.  This means that we\n> potentially print these values incorrectly if they are greater than\n> INT_MAX.\n> \n> This has been the case since the --debug option was introduced in 'git\n> ls-files' in 8497421715 (\"ls-files: learn a debugging dump format\",\n> 2010-07-31).\n\nI didn't see any comment on this, but it seems like it must be obviously\ncorrect, since as you note we do define those fields as unsigned. I'm\nreally surprised that -Wformat doesn't catch this, though. I wonder why.\n\n-Peff\n"},{"id":"373691","messageId":"20190411212830.GF32487@hank.intra.tgummerer.com","threadId":"50889","inReplyTo":"20190411041823.GA17699@sigill.intra.peff.net","subject":"Re: [PATCH] ls-files: use correct format string","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-04-11T21:28:30Z","receivedAt":"2019-04-11T21:28:35Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 04/11, Jeff King wrote:\n> On Sun, Apr 07, 2019 at 07:47:51PM +0100, Thomas Gummerer wrote:\n> \n> > struct stat_data and struct cache_time both use unsigned ints for all\n> > their members.  However the format string for 'git ls-files --debug'\n> > currently uses %d for formatting these numbers.  This means that we\n> > potentially print these values incorrectly if they are greater than\n> > INT_MAX.\n> > \n> > This has been the case since the --debug option was introduced in 'git\n> > ls-files' in 8497421715 (\"ls-files: learn a debugging dump format\",\n> > 2010-07-31).\n> \n> I didn't see any comment on this, but it seems like it must be obviously\n> correct, since as you note we do define those fields as unsigned. I'm\n> really surprised that -Wformat doesn't catch this, though. I wonder why.\n\nGood point.  A bit of digging led me to -Wformat-signedness, which\nshould catch this.  This turns up a lot of errors in our codebase.  I\ndidn't go through to see how many of them are actual errors, and how\nmany are false-positives though.\n\nhttps://gcc.gnu.org/bugzilla/show_bug.cgi?id=65446 describes how the\noption can lead to false positives, e.g.\n\n    printf (\"%u\\n\", unsigned_short);\n\nmight turn up an error.  From a quick test this seems to work\ncorrectly with gcc 8.2.1 that I have on my machine though, so the\nissue might be fixed in newer gcc version, even though that bug report\nis still marked as new.\n\nMaybe it's worth going through the warnings at some point to see if it\nwould be possible to turn -Wformat-signedness on.\n"},{"id":"373694","messageId":"20190411234920.GA27914@sigill.intra.peff.net","threadId":"50889","inReplyTo":"20190411212830.GF32487@hank.intra.tgummerer.com","subject":"Re: [PATCH] ls-files: use correct format string","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-11T23:49:20Z","receivedAt":"2019-04-11T23:49:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 11, 2019 at 10:28:30PM +0100, Thomas Gummerer wrote:\n\n> > I didn't see any comment on this, but it seems like it must be obviously\n> > correct, since as you note we do define those fields as unsigned. I'm\n> > really surprised that -Wformat doesn't catch this, though. I wonder why.\n> \n> Good point.  A bit of digging led me to -Wformat-signedness, which\n> should catch this.  This turns up a lot of errors in our codebase.  I\n> didn't go through to see how many of them are actual errors, and how\n> many are false-positives though.\n\nAh, right, I totally forgot that signedness got its own warning class.\nThanks for enlightening me.\n\n> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65446 describes how the\n> option can lead to false positives, e.g.\n> \n>     printf (\"%u\\n\", unsigned_short);\n> \n> might turn up an error.  From a quick test this seems to work\n> correctly with gcc 8.2.1 that I have on my machine though, so the\n> issue might be fixed in newer gcc version, even though that bug report\n> is still marked as new.\n\nInteresting. Looking at that thread, I actually don't think it would be\nso bad to warn there anyway. It's true that due to integer promotion an\nunsigned short will work with %u, but I'd be just as happy to switch\nsuch a format to \"%hu\", which is more correct.\n\n> Maybe it's worth going through the warnings at some point to see if it\n> would be possible to turn -Wformat-signedness on.\n\nI skimmed over a few of the results. There are definitely some that\ncould produce funny output. There are also many that are harmless (e.g.,\nprinting a constant 0 with \"%o\", which technically should be \"0U\"). I\ndon't think it's high priority, but if anybody wants to chip away at it,\nbe my guest.\n\nIn the meantime, I think your patch here is an obvious improvement.\n\n-Peff\n"}]}