Re: [PATCH v1 3/3] convert: add filter.<driver>.useProtocol option
- From
- Eric Wong <e@80x24.org>
- Date
- Jul 25, 2016, 07:27 UTC
- Message-ID
- <20160725072745.GB11634@starla>
- In-Reply-To
- <121F8453-3A12-47AE-AE10-021EADC4D995@gmail.com>
Lars Schneider <larsxschneider@gmail.com> wrote:
Show 14 quoted lines
> 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>
Show 8 quoted lines
> >> + 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.