Re: Netsurf build failure: 'PATH_MAX' undeclared (patch)
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
Reply to: