Hi! On Sat, 2021-05-29 at 19:16:57 +0100, João Pedro Malhado wrote: > I don't know if upstream would accept such a patch as they need to worry about > platforms like RISC OS and AmigaOS, and I don't know if this would work there. > It might be ok for Debian though. If there are no objections/corrections I > could > try upstream first.
In general, I'd recommend switching all code bases to use dynamic allocation for pathname handling. > --- netsurf-3.10.orig/libnsutils/src/time.c > +++ netsurf-3.10/libnsutils/src/time.c > @@ -16,11 +16,11 @@ > #include <stdlib.h> > #include <unistd.h> > > -#if (defined(_POSIX_TIMERS) && (_POSIX_TIMERS > 0) && (defined > _POSIX_MONOTONIC_CLOCK)) || defined(__OpenBSD__) > +#if (defined(_POSIX_TIMERS) && (_POSIX_TIMERS > 0) && (defined > _POSIX_MONOTONIC_CLOCK)) || defined(__OpenBSD__) || defined(__GNU__) Does the Hurd not define _POSIX_TIMERS > 0 and _POSIX_MONOTONIC_CLOCK? > #include <time.h> > #elif defined(__riscos) > #include <oslib/os.h> > -#elif defined(__MACH__) > +#elif defined(__MACH__) && defined(__APPLE__) > #include <mach/mach.h> > #include <mach/clock.h> > #include <mach/mach_time.h> > --- netsurf-3.10.orig/netsurf/frontends/framebuffer/fetch.c > +++ netsurf-3.10/netsurf/frontends/framebuffer/fetch.c > @@ -48,13 +48,17 @@ > */ > static nsurl *get_resource_url(const char *path) > { > - char buf[PATH_MAX]; > + char *buf=NULL; > nsurl *url = NULL; Always try to mimic the existing coding style. In this case spaces around ‘=’. > if (strcmp(path, "favicon.ico") == 0) > path = "favicon.png"; > > - netsurf_path_to_nsurl(filepath_sfind(respaths, buf, path), &url); > + netsurf_path_to_nsurl(filepath_sfind(respaths, &buf, path), &url); > + ^ Watch for those trailing spaces. > + if (buf != NULL) { > + free(buf); > + } There's no need to check for NULL before calling free(). > > return url; > } > --- netsurf-3.10.orig/netsurf/frontends/framebuffer/font_freetype.c > +++ netsurf-3.10/netsurf/frontends/framebuffer/font_freetype.c > @@ -120,15 +120,18 @@ fb_new_face(const char *option, const ch > fb_faceid_t *newf; > FT_Error error; > FT_Face aface; > - char buf[PATH_MAX]; > + char *buf=NULL; Spaces around ‘=’. > > newf = calloc(1, sizeof(fb_faceid_t)); > > if (option != NULL) { > newf->fontfile = strdup(option); > } else { > - filepath_sfind(respaths, buf, fontname); > + filepath_sfind(respaths, &buf, fontname); > newf->fontfile = strdup(buf); > + if (buf != NULL) { > + free(buf); > + } No NULL check. > } > > error = FTC_Manager_LookupFace(ft_cmanager, (FTC_FaceID)newf, > &aface); > --- netsurf-3.10.orig/netsurf/frontends/gtk/fetch.c > +++ netsurf-3.10/netsurf/frontends/gtk/fetch.c > @@ -249,14 +249,17 @@ const char *fetch_filetype(const char *u > > static nsurl *nsgtk_get_resource_url(const char *path) > { > - char buf[PATH_MAX]; > + char *buf = NULL; > nsurl *url = NULL; > > /* favicon.ico -> favicon.png */ > if (strcmp(path, "favicon.ico") == 0) { > nsurl_create("resource:favicon.png", &url); > } else { > - netsurf_path_to_nsurl(filepath_sfind(respaths, buf, path), > &url); > + netsurf_path_to_nsurl(filepath_sfind(respaths, &buf, path), > &url); > + if (buf != NULL) { > + free(buf); > + } No NULL check. > } > > return url; > --- netsurf-3.10.orig/netsurf/frontends/gtk/gui.c > +++ netsurf-3.10/netsurf/frontends/gtk/gui.c > @@ -335,8 +335,8 @@ static nserror nsgtk_add_named_icons_to_ > */ > static nserror nsgtk_init(int argc, char** argv, char **respath) > { > - char buf[PATH_MAX]; > - char *resource_filename; > + char *buf = NULL; > + char *resource_filename = NULL; > char *addr = NULL; > nsurl *url; > nserror res; > @@ -407,8 +407,12 @@ static nserror nsgtk_init(int argc, char > browser_set_dpi(gdk_screen_get_resolution(gdk_screen_get_default())); > NSLOG(netsurf, INFO, "Set CSS DPI to %d", browser_get_dpi()); > > - filepath_sfinddef(respath, buf, "mime.types", "/etc/"); > + filepath_sfinddef(respath, &buf, "mime.types", "/etc/"); > gtk_fetch_filetype_init(buf); > + > + if (buf != NULL) { > + free(buf); > + } No NULL check. > > save_complete_init(); > > --- netsurf-3.10.orig/netsurf/frontends/monkey/fetch.c > +++ netsurf-3.10/netsurf/frontends/monkey/fetch.c > @@ -36,10 +36,13 @@ extern char **respaths; > > static nsurl *gui_get_resource_url(const char *path) > { > - char buf[PATH_MAX]; > + char *buf=NULL; Spaces around ‘=’. > nsurl *url = NULL; > > - netsurf_path_to_nsurl(filepath_sfind(respaths, buf, path), &url); > + netsurf_path_to_nsurl(filepath_sfind(respaths, &buf, path), &url); > + if (buf != NULL) { > + free(buf); > + } No NULL check. > > return url; > } > --- netsurf-3.10.orig/netsurf/frontends/monkey/main.c > +++ netsurf-3.10/netsurf/frontends/monkey/main.c > @@ -379,7 +379,7 @@ main(int argc, char **argv) > { > char *messages; > char *options; > - char buf[PATH_MAX]; > + char *buf=NULL; Spaces around ‘=’. > nserror ret; > struct netsurf_table monkey_table = { > .misc = &monkey_misc_table, > @@ -441,8 +441,11 @@ main(int argc, char **argv) > die("NetSurf failed to initialise"); > } > > - filepath_sfinddef(respaths, buf, "mime.types", "/etc/"); > + filepath_sfinddef(respaths, &buf, "mime.types", "/etc/"); > monkey_fetch_filetype_init(buf); > + if (buf != NULL) { > + free(buf); > + } No NULL check. > > urldb_load(nsoption_charp(url_file)); > urldb_load_cookies(nsoption_charp(cookie_file)); > --- netsurf-3.10.orig/netsurf/frontends/windows/fetch.c > +++ netsurf-3.10/netsurf/frontends/windows/fetch.c > @@ -76,10 +76,13 @@ static const char *fetch_filetype(const > */ > static nsurl *nsw32_get_resource_url(const char *path) > { > - char buf[PATH_MAX]; > + char *buf = NULL; > nsurl *url = NULL; > > - netsurf_path_to_nsurl(filepath_sfind(G_resource_pathv, buf, path), > &url); > + netsurf_path_to_nsurl(filepath_sfind(G_resource_pathv, &buf, path), > &url); > + if (buf != NULL) { > + free(buf); > + } No NULL check. > > return url; > } > --- netsurf-3.10.orig/netsurf/frontends/windows/main.c > +++ netsurf-3.10/netsurf/frontends/windows/main.c > @@ -191,7 +191,11 @@ static nserror set_defaults(struct nsopt > if (res_len > 0) { > nsoption_setnull_charp(ca_bundle, strdup(buf)); > } else { > - ptr = filepath_sfind(G_resource_pathv, buf, "ca-bundle.crt"); > + if (buf != NULL) { > + free(buf); No NULL check. > + buf = NULL; > + } > + ptr = filepath_sfind(G_resource_pathv, &buf, "ca-bundle.crt"); > if (ptr != NULL) { > nsoption_setnull_charp(ca_bundle, strdup(buf)); > } > @@ -204,6 +208,7 @@ static nserror set_defaults(struct nsopt > * not available so use the obsolete method of user prodile > * with downloads suffixed > */ > + buf = realloc(buf,buf_bytes_size); This can leak on failure. You need a temporary variable to hold the return value and check whether it's not NULL, and then either free the other or assign the new value on top, and perhaps abort as needed or rollback or something. Missing space after ‘,’. > buf[0] = '\0'; > > hres = SHGetFolderPath(NULL, > --- netsurf-3.10.orig/netsurf/utils/filepath.c > +++ netsurf-3.10/netsurf/utils/filepath.c > @@ -24,6 +24,11 @@ > * straightforward. > */ > > +/* This is needed by asprinf type of functions on GNU/Hurd*/ Typo “asprintf”, I guess you also need a “. ” before the comment end. > +#if defined(__GNU__) > +#define _GNU_SOURCE 1 > +#endif > + Hmm, given that you are using asprintf family functions unconditionally, something like this will be needed for all architectures not just GNU/Hurd. This also seems like it should be defined in the build-system instead. > #include <sys/types.h> > #include <sys/stat.h> > #include <stdarg.h> > @@ -42,44 +47,43 @@ > #define MAX_RESPATH 128 > > /* exported interface documented in filepath.h */ > -char *filepath_vsfindfile(char *str, const char *format, va_list ap) > +char *filepath_vsfindfile(char **str, const char *format, va_list ap) > { > - char *realpathname; > char *pathname; > int len; > > - pathname = malloc(PATH_MAX); > - if (pathname == NULL) > - return NULL; /* unable to allocate memory */ > - > - len = vsnprintf(pathname, PATH_MAX, format, ap); > - > - if ((len < 0) || (len >= PATH_MAX)) { > - /* error or output exceeded PATH_MAX length so > - * operation is doomed to fail. > - */ > - free(pathname); > + if (str == NULL) { > return NULL; > + } The coding style, seems to prefer omitting the curly brackets for single statements. > + > + len = vasprintf(&pathname, format, ap); > + > + if (len < 0) { > + return NULL; /* error in vasprintf */ > } Excess curly brackets. > > - realpathname = realpath(pathname, str); > + *str = realpath(pathname, NULL); I'd probably only assign into *str the final pathname, after the function has made sure everything is fine. And probably would also assign NULL at the beginning, so that on any error condition *str has a consistent state. To make this all more future-proof. > > - free(pathname); > + if (pathname != NULL) { > + free(pathname); > + } No NULL check. > > - if (realpathname != NULL) { > + if (*str != NULL) { > /* sucessfully expanded pathname */ > - if (access(realpathname, R_OK) != 0) { > + if (access(*str, R_OK) != 0) { > /* unable to read the file */ > + free(*str); > + *str=NULL; This would not be needed here, neither the missing ones on the other early returns. > return NULL; > } > } > Here we'd also assign into *str. And I'd probably leave the existing realpathname handling as is. Actually, seeing now that str was only ever used to be filled with the contents of the result, and now we always allocate it anyway, I'd probably just get rid of the function argument completely. > - return realpathname; > + return *str; > } > > > /* exported interface documented in filepath.h */ > -char *filepath_sfindfile(char *str, const char *format, ...) > +char *filepath_sfindfile(char **str, const char *format, ...) I'm assuming this str argument can also go. > { > va_list ap; > char *ret; > @@ -105,8 +109,9 @@ char *filepath_findfile(const char *form > return ret; > } > > + > /* exported interface documented in filepath.h */ > -char *filepath_sfind(char **respathv, char *filepath, const char *filename) > +char *filepath_sfind(char **respathv, char **filepath, const char *filename) The same as with filepath here? > { > int respathc = 0; > > @@ -115,7 +120,7 @@ char *filepath_sfind(char **respathv, ch > > while (respathv[respathc] != NULL) { > if (filepath_sfindfile(filepath, "%s/%s", respathv[respathc], > filename) != NULL) { > - return filepath; > + return *filepath; > } > > respathc++; > @@ -128,33 +133,23 @@ char *filepath_sfind(char **respathv, ch > /* exported interface documented in filepath.h */ > char *filepath_find(char **respathv, const char *filename) > { > - char *ret; > - char *filepath; > + char *filepath = NULL; > > if ((respathv == NULL) || (respathv[0] == NULL)) > return NULL; > > - filepath = malloc(PATH_MAX); > - if (filepath == NULL) > - return NULL; > - > - ret = filepath_sfind(respathv, filepath, filename); > - > - if (ret == NULL) > - free(filepath); > - > - return ret; > + return filepath_sfind(respathv, &filepath, filename); > } > > > /* exported interface documented in filepath.h */ > char * > filepath_sfinddef(char **respathv, > - char *filepath, > + char **filepath, > const char *filename, > const char *def) > { And filepath here too. > - char t[PATH_MAX]; > + char *t = NULL; While the spaces vs tabs handling in the upstream code does not seem consistent, I'd preserve the style on changed lines. > char *ret; > > if ((respathv == NULL) || (respathv[0] == NULL) || (filepath == NULL)) > @@ -164,17 +159,22 @@ filepath_sfinddef(char **respathv, > > if ((ret == NULL) && (def != NULL)) { > /* search failed, return the path specified */ > - ret = filepath; > + ret = *filepath; > if (def[0] == '~') { > - snprintf(t, PATH_MAX, "%s/%s/%s", getenv("HOME"), def + > 1, filename); > + asprintf(&t, "%s/%s/%s", getenv("HOME"), def + 1, > filename); > } else { > - snprintf(t, PATH_MAX, "%s/%s", def, filename); > + asprintf(&t, "%s/%s", def, filename); > } > if (realpath(t, ret) == NULL) { > - strncpy(ret, t, PATH_MAX); > + strncpy(ret, t, strlen(ret)); > } The strlen() here seems bogus, as it should depend on t not ret, but in any case the whole premise in this block now seems fishy. ret here should ideally be just NULL, otherwise we do not know if realpath() will have enough space for its copy. And then on failure we'd instead simply assign t to ret, as that's already been assigned with the correct allocated spaces. > > } > + > + if (t != NULL) { > + free(t); > + } > + No NULL check. Although if we are going to return t, we should only free() it when realpath() has succeeded. > return ret; > } > Thanks, Guillem