GNOME Bugzilla – Bug 603997
Unconditional use of PATH_MAX causes FTBFS
Last modified: 2014-08-28 12:48:05 UTC
Hi, The multiload applet unconditionally uses PATH_MAX, which is not guaranteed to be defined by POSIX, and causes a build failure on platforms that don't define it. This happens e.g. on GNU/Hurd. The attached patch changes the code to use dynamic allocation to achieve the same result, making the code more portable and secure.
Created attachment 149270 [details] [review] [PATCH] Bug 603997: Don't unconditionally use PATH_MAX Subject: [PATCH] Bug 603997: Don't unconditionally use PATH_MAX Since PATH_MAX is not guaranteed to be defined, unconditionally using it will cause a build failure on platforms that don't define it. So stop using it and use dynamic allocation to achieve the same result.
You're malloc()ing a string pointer, then writing to it and then assuming that string will have a certain content without checking that the malloc actually worked. Sounds like somewhere between a security flaw and an unpredictable runtime result. Does Hurd have MAXPATHLEN (see Bug #454777 solution to similar problem in a different package)?
Created attachment 149322 [details] [review] [PATCH] Bug 603997: Don't unconditionally use PATH_MAX, take 2 (In reply to comment #2) > You're malloc()ing a string pointer, then writing to it and then assuming that > string will have a certain content without checking that the malloc actually > worked. Sounds like somewhere between a security flaw and an unpredictable > runtime result. Oh right. Well if the malloc fails, it will return NULL, and then we will segfault when passing NULL to sprintf... and we don't want that! > Does Hurd have MAXPATHLEN (see Bug #454777 solution to similar > problem in a different package)? It doesn't. Though that bug is the other way round AFAICS (they were using MAXPATHLEN and look for PATH_MAX now), but none is guaranteed to be defined, since there may be no path limit :) This new patch checks for the malloc return value and returns FALSE if malloc fails to allocate memory.
So does this look right now? Can we get it in for 2.29.4?
I'd love to get this patch in before 2.30. Can somebody review it?
Ping? Can I commit this?
I think missing "%s" in strlen ("/sys/class/net//device") in the malloc call.
(In reply to comment #7) > I think missing "%s" in strlen ("/sys/class/net//device") in the malloc call. That's intentional, since that's a strlen("/sys/class/net//device") + strlen(device), where device would be the %s that is missing. That's since it's for a malloc, the sprintf calls later are fine and have enough space thanks to the above code.
Oops, yeah, now I see what's happening there. All looks sensible to me.
(In reply to comment #9) > Oops, yeah, now I see what's happening there. All looks sensible to me. Is that an 'ok to push'?
I don't have commit-bits and I'm not a gnome-applets developer lead, so I don't have actual power to push. But I would if I did.
I committed your previous patch as I took it from debian and only then looked at this bug.
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.