[FFmpeg-devel] [PATCH v4 1/1] avformat: Add IPFS protocol support.

[Tomas Härdin]

fre 2022-02-04 klockan 15:12 +0100 skrev Mark Gaiser:
> On Fri, Feb 4, 2022 at 12:10 PM Tomas Härdin <tjoppen at acc.umu.se>
> wrote:
> 
> > tor 2022-02-03 klockan 18:29 +0100 skrev Mark Gaiser:
> > > 
> > > +typedef struct IPFSGatewayContext {
> > > +    AVClass *class;
> > > +    URLContext *inner;
> > > +    char *gateway;
> > 
> > Consider two separate variables. One for AVOption and one for the
> > dynamically allocated string. Or put the latter on the stack.
> > 
> 
> There always needs to be a gateway so why is reusing that variable an
> issue?
> I'm fine splitting it up but I'd like to understand the benefit of it
> as
> currently I don't see that benefit.

Because of the way AVOption memory allocation works

> 
> > > +static int populate_ipfs_gateway(URLContext *h)
> > > +{
> > > +    IPFSGatewayContext *c = h->priv_data;
> > > +    char *ipfs_full_data_folder = NULL;
> > > +    char *ipfs_gateway_file = NULL;
> > 
> > These can be char[PATH_MAX]
> > 
> 
> Oke, will do.
> C code question though.
> How do I use av_asprintf on stack arrays like that?

snprintf(). Also be careful with PATH_MAX and +-1 bytes for the NUL.

> 
> > Again, there is no reason to stat this. Just try opening the
> > gateway
> > file directly.
> > 
> 
> This is a folder, not a file.
> 
> The other stat that was here too was a file, I replaced that with an
> fopen.
> It smells sketchy to me to (ab)use fopen to check if a folder exists.
> There's stat for that.

You don't need to check whether the folder exists at all. The only
thing that accomplishes is some AV_LOG_DEBUG prints that won't even get
compiled in unless a users builds with -g (I think). It's not sketchy -
it's spec'd behavior.

> 
> 
> > 
> > > +
> > > +    // Read a single line (fgets stops at new line mark).
> > > +    fgets(gateway_file_data, sizeof(gateway_file_data) - 1,
> > > gateway_file);
> > 
> > This can result in gateway_file_data not being NUL terminated
> 
> 
> > > +
> > > +    // Replace first occurence of end of line to \0
> > > +    gateway_file_data[strcspn(gateway_file_data, "\r\n")] = 0;
> > 
> > What if the file uses \n or no newlines at all?
> > 
> 
> Right.
> So I guess the fix here is:
> 1. Initialize gateway_file_data so all bytes are zero
> 2. read a line
> 3. set the last byte of gateway_file_data to 0
> 
> Now any text in the string will be the gateway.
> 
> Is that a proper fix?

Yes always putting a NUL at the end works. You don't need to initialize
with zero in that case. fgets() will NUL terminate except when there's
an error like the line being too long.

> 
> 
> > > +err:
> > > +    if (gateway_file)
> > > +        fclose(gateway_file);
> > > +
> > > +    av_free(ipfs_full_data_folder);
> > > +    av_free(ipfs_gateway_file);
> > 
> > This is not cleaning up dynamic allocations of c->gateway
> > 
> 
> So I should do that in  ipfs_close, right?

That's one place to do it yes. I forget whether _close() is called in
case of errors. av_freep() will set the pointer to NULL after freeing
so no double-frees occur.

> 
> > 
> > 
> > > +    // Test if the gateway starts with either http:// or
> > > https://
> > > +    // The remainder is stored in url_without_protocol
> > > +    if (av_stristart(uri, "http://", &url_without_protocol) == 0
> > > +        && av_stristart(uri, "https://", &url_without_protocol)
> > > ==
> > > 0) {
> > > +        av_log(h, AV_LOG_ERROR, "The gateway URL didn't start
> > > with
> > > http:// or https:// and is therefore invalid.\n");
> > > +        ret = -2;
> > > +        goto err;
> > > +    }
> > 
> > I guess restricting this to HTTP schemes is OK. Or are there non-
> > HTTP
> > gateways for this?
> > 
> 
> No.
> At least not from the IPFS camp.
> The IPFS software creates a gateway and that is specifically an http
> gateway.
> Users can put that behind a proxy making it (potentially) a https
> gateway
> but that's about it.

I see. I guess if any user puts this stuff behind gopher:// or
something then that's their problem.

> 
> > 
> > > +    if (last_gateway_char != '/') {
> > > +        c->gateway = av_asprintf("%s/", c->gateway);
> > 
> > Yet another leak
> > 
> 
> Please tell me how to fix this one.
> As you can see, I need the c->gateway value to copy and add a "/" to
> it.
> 
> In C++ this would just be a dead simple append ;)

Ensure there's enough space for '/' and a NUL and just write that to
the end.

snprintf() can do all of this if used appropriately. For example to
conditionally append "/" you can put %s in the format string and the
ternary

 needs_slash ? "/" : ""

as the associated argument

/Tomas