fre 2022-02-04 klockan 15:12 +0100 skrev Mark Gaiser: > On Fri, Feb 4, 2022 at 12:10 PM Tomas Härdin <tjop...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".