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

[Mark Gaiser]

On Mon, Feb 7, 2022 at 4:09 PM Tomas Härdin <tjoppen at acc.umu.se> wrote:

> 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
>
>
Thank you so much for all the valuable feedback! It's much appreciated :)

I'll fix all the still open issues and send another patch mail somewhere
later this week.
Question though, as this seems to be heading towards the final patch
version. How do you prefer to see the next patch round?

1. As it's done currently. You don't see what's different.
2. As a 1/2 and 2/2 patch where 2/2 would be the a diff on top of 1/2 with
the review feedback applied.

Either is fine with me.
But I fear that a split patch mail (so 1/2 with the code as is right now
and 2/2 with the feedback changes) could potentially waste people's time if
they comment on something that is fixed in the feedback patch.


> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>