Re: [PATCH v1 3/3] convert: add filter.<driver>.useProtocol option

Lars Schneider [off-list ref] wrote:

On 23 Jul 2016, at 10:14, Eric Wong [off-list ref] wrote:

quoted

larsxschneider@gmail.com wrote:

quoted

+static struct cmd2process *start_protocol_filter(const char *cmd)
+{
+	int ret = 1;
+	struct cmd2process *entry = NULL;
+	struct child_process *process = NULL;

These are unconditionally set below, so initializing to NULL
may hide future bugs.

OK. I thought it is generally a good thing to initialize a pointer with 
NULL. Can you explain to me how this might hide future bugs?
I will remove the initialization.

Compilers complain about uninitialized variables.  Blindly
setting them to NULL can allow them to be dereferenced;
triggering segfaults; especially if it's passed to a different
compilation unit the compiler can't see.

quoted

quoted

+static int apply_protocol_filter(const char *path, const char *src, size_t len,
+						int fd, struct strbuf *dst, const char *cmd,
+						const char *filter_type)
+{

<snip>

quoted

quoted

+			if (fd >= 0 && !src) {
+				ret &= fstat(fd, &fileStat) != -1;
+				len = fileStat.st_size;

There's a truncation bug when sizeof(size_t) < sizeof(off_t)

OK. What would you suggest to do in that case? Should we just let the
filter fail? Is there anything else we could do?

Anything which refers to something on disk (or will eventually
stored there, such as blobs) should evolve towards off_t rather
than size_t.  We just discovered a bunch of 32-bit truncation
bugs the other week:

https://public-inbox.org/git/1466807902.28869.8.camel@gmail.com/

If the protocol/ABI is frozen, it should probably fail;
and a 64-bit-off_t version for 32-bit systems should be defined.