[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] libxl: Be more careful with error handling in libxl__dm_runas_helper()
On 11/27/2015 04:40 AM, Ian Campbell wrote: On Thu, 2015-11-26 at 17:45 +0000, Ian Jackson wrote:Boris Ostrovsky writes ("[PATCH] libxl: Be more careful with error handling in libxl__dm_runas_helper()"):getpwnam_r() has fairly complicated return rules. From man pages: RETURN VALUE ... On success, getpwnam_r() and getpwuid_r() return zero, and set *result to pwd. If no matching password record was found, these functions return 0 and store NULL in *result. In case of error, an error number is returned, and NULL is stored in *result.I can't see anything in the SuS docs (which Ian C referred to) saying that *result is updated even on error. So I think you need to check the return value first, and only then *result.Actually, the fourth para of description in [0] (starting "The getpwnam_r() function shall...") ends: A null pointer shall be returned at the location pointed to by result on error or if the requested entry is not found. So this appears to be common with Linux implementation(s) and therefore is what we should use as indication of "non-success". After that I think we can state that the entry was not found (bit not a "real" error occurred) if (!ret || (ret==ENOENT) || (ret==ESRCH) || (errno==ENOENT) || (errno==ESRCH)) based on this statement from man pages' NOTES:The formulation given above under "RETURN VALUE" is from POSIX.1-2001. It does not call "not found" an error, and hence does not specify what value errno might have in this situation. But that makes it impossible to recognize errors. One might argue that according to POSIX errno should be left unchanged if an entry is not found.(followed by what IanC quotes below --- something that I missed during my first reading) To answer the question asked earlier in this thread --- the two systems I tested this on are Fedora 18 and 20. This piece of code: errno = 0; s = getpwnam_r(argv[1], &pwd, buf, bufsize, &result); printf("s = %d errno = %d result = %p\n", s, errno, result); if (result == NULL) { if (s == 0) printf("Not found\n"); else { errno = s; perror("getpwnam_r"); } exit(EXIT_FAILURE); } results in: root@haswell> cat /etc/redhat-release Fedora release 18 (Spherical Cow) root@haswell> ./a.out foobar s = 0 errno = 0 result = (nil) Not found root@haswell> and root@orochi-c> cat /etc/redhat-release Fedora release 20 (Heisenbug) root@orochi-c> ./a.out foobar s = 2 errno = 2 result = (nil) getpwnam_r: No such file or directory root@orochi-c>which does look like a bug in F20 since the code above is taken from man pages' EXAMPLE and doesn't look like it works as intended (which was to produce "Not found" string) -boris I also just realised (long after everyone else, apparently) that this function returns (+ve) Exxx values, not -1 and setting errno. Together with combining getpwnam and getpwnam_r in the same doc it's almost like someone was wilfully trying to make this function difficult to figure out...ERRORS 0 or ENOENT or ESRCH or EBADF or EPERM or ... The given name or uid was not found. While it's not clear what ellipses are meant to be, the way we currently treat return values from getpwnam_r() is no sufficient. In fact, two of my systems behave differently when username is not found: one returns ENOENT and the other returns 0. Both set *result to NULL.I don't know where all that stuff about ENOENT comes from. I'm tempted to say this is a bug in your C library. But I don't mind treating ENOENT or ESRCH as ERROR_NOTFOUND. I do mind treating EBADF or EPERM that way.FWIW the Linux manpage which Boris has referred to says under NOTES: Experiments on various UNIX-like systems show that lots of different values occur in this situation: 0, ENOENT, EBADF, ESRCH, EWOULDBLOCK, EPERM, and probably others "various" probably includes all sorts of random UNIX-like systems outside the small set which are actually used with Xen. I'd be inclined to agree with not including EBADF, EPERM in this way at least until an actual platform running Xen is found which returns such errors (not suggesting anyone proactively checking, but just that we should be prepared to accept the possibility when someone notices and reports it). Ian. [0] http://pubs.opengroup.org/onlinepubs/9699919799/functions/getpwnam.htmlWhile at it, also make sure we don't get stuck on ERANGE.If you are going to do anything to this, you should use an exponentially increasing buffer size. Bear in mind that getpwnam_r is already inside the trust boundary. Thanks, Ian. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |