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

[Eric Wong <e@80x24.org>]

Lars Schneider <larsxschneider@gmail.com> wrote:
> On 23 Jul 2016, at 10:14, Eric Wong <e@80x24.org> wrote:
> > larsxschneider@gmail.com wrote:
> >> +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.

> >> +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>

> >> +			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.