| Summary: | Improper use of asprintf() in login/pam_systemd.c | ||
|---|---|---|---|
| Product: | systemd | Reporter: | Archie Cobbs <archie.cobbs> |
| Component: | general | Assignee: | systemd-bugs |
| Status: | RESOLVED WONTFIX | QA Contact: | systemd-bugs |
| Severity: | normal | ||
| Priority: | medium | CC: | KOTI-P-scholar |
| Version: | unspecified | ||
| Hardware: | Other | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
asprintf() is not POSIX anyway. This has come up before, but I think the man page is simply a bit unlcear (I mean, it confuses "undefined" with "whatever was set before", the code in glibc actually doesn't clobber the pointer. Either way, I am pretty sure our code does this right and we should leave it that way. SHould asprintf() in glibc really change their logic one day and start clobbering the pointer on failure, then the right answer is to wrap asprintf() in some call safe_asprintf() or so which fixes this behaviour. Either way I don't think we need to fix anything now. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.
Reviewing this code: https://github.com/systemd/systemd/blob/master/src/login/pam_systemd.c#L179-190 there appears to be a bug. Here is the code: #ifdef ENABLE_KDBUS _cleanup_free_ char *s = NULL; int r; /* skip export if kdbus is not active */ if (access("/sys/fs/kdbus", F_OK) < 0) return PAM_SUCCESS; if (asprintf(&s, KERNEL_USER_BUS_ADDRESS_FMT ";" UNIX_USER_BUS_ADDRESS_FMT, uid, runtime) < 0) { pam_syslog(handle, LOG_ERR, "Failed to set bus variable."); return PAM_BUF_ERR; } The bug occurs if asprintf() fails. Since "s" is declared _cleanup_free, it will be automatically free()'d when the function returns (right?). However, there is no guarantee that "s" will still be equal to NULL at this point, as the code incorrectly assumes. Quoting the asprintf() man page: RETURN VALUE When successful, these functions return the number of bytes printed, just like sprintf(3). If memory allocation wasn't possible, or some other error occurs, these functions will return -1, and the contents of strp is undefined. ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This is only one example; there could be arbitrarily many others in the systemd codebase (I haven't looked). In practice, this bug probably never happens, because I doubt that any actual asprintf() implementations modify "s" on failure. But that's irrelevant to whether this is a bug. Personally I think it's stupid that POSIX allows asprintf() to modify *strp on failure, but whatever, it's too late to fix that now.