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

[Mark Gaiser <markg85@gmail.com>]

On Wed, Feb 9, 2022 at 6:36 PM Mark Gaiser <markg85@gmail.com> wrote:

> On Mon, Feb 7, 2022 at 4:09 PM Tomas Härdin <tjoppen@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@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.
>
>
Never mind the question. V5 is up as option 1. This change again touched
about half the file so a diff doesn't make sense.


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

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".