[PATCH libinput v2] Fix to avoid use of undefined pointer values.
Peter Hutterer
peter.hutterer at who-t.net
Sun May 31 16:18:46 PDT 2015
On Fri, May 29, 2015 at 10:40:54AM -0700, Jon A. Cruz wrote:
> Yes. The man page calls out its behavior.
>
> http://linux.die.net/man/3/asprintf
>
> > 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.
>
and a quick check of fedora's glibc shows there are two possible return
statements before _IO_vasprintf touches the result pointer, including the
one where vfprintf fails. That's "untouched" rather than "undefined" but
either way would've been a bug in the original code here.
Jon: I split the config.h includes into a separate patch and added the
printf attribute. Pushed otherwise, thanks.
Cheers,
Peter
> On 05/29/2015 10:38 AM, Jasper St. Pierre wrote:
> > I can't find any standard for asprintf outside of the glibc library,
> > which doesn't mention anything about undefined behavior. Am I missing
> > something?
> >
> > http://www.gnu.org/software/libc/manual/html_node/Dynamic-Output.html
> >
> > On Fri, May 29, 2015 at 10:29 AM, Jon A. Cruz <jonc at osg.samsung.com> wrote:
> >> Well, no.
> >>
> >> It is clearly documented to be "undefined", so it is completely within
> >> spec for it to be just about anything. One of the more likely bad
> >> scenarios is that the call internally allocates a buffer, starts to use
> >> it, hits some problem, then frees the buffer while leaving its address
> >> in place. Use of stale pointers should always be avoided.
> >>
> >> On 05/29/2015 09:48 AM, Bill Spitzak wrote:
> >>> I'm pretty certain the only two things that can happen is that str is
> >>> unchanged or it is set to NULL. So presetting it to NULL would work.
> >>>
> >>>
> >>> On Thu, May 28, 2015 at 7:01 PM, Jon A. Cruz <jonc at osg.samsung.com> wrote:
> >>>
> >>>> If asprintf fails for any reason, the contents of the pointer
> >>>> are undefined. While some platforms set it to NULL, there is no
> >>>> guarantee that all will.
> >>>>
> >>>> This change adds a simple wrapper to ensure proper NULL results
> >>>> on failure.
> >>>>
> >>>> Signed-off-by: Jon A. Cruz <jonc at osg.samsung.com>
> >>>> ---
> >>>> src/evdev-middle-button.c | 2 ++
> >>>> src/evdev-mt-touchpad-buttons.c | 2 ++
> >>>> src/evdev-mt-touchpad-edge-scroll.c | 2 ++
> >>>> src/libinput-util.h | 30 ++++++++++++++++++++++++++++++
> >>>> src/timer.c | 2 ++
> >>>> test/litest.c | 2 +-
> >>>> tools/libinput-list-devices.c | 14 +++++++-------
> >>>> 7 files changed, 46 insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/src/evdev-middle-button.c b/src/evdev-middle-button.c
> >>>> index 903fa4d..c989486 100644
> >>>> --- a/src/evdev-middle-button.c
> >>>> +++ b/src/evdev-middle-button.c
> >>>> @@ -20,6 +20,8 @@
> >>>> * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> >>>> */
> >>>>
> >>>> +#include "config.h"
> >>>> +
> >>>> #include <stdint.h>
> >>>>
> >>>> #include "evdev.h"
> >>>> diff --git a/src/evdev-mt-touchpad-buttons.c
> >>>> b/src/evdev-mt-touchpad-buttons.c
> >>>> index 64e9d28..4b1d6ab 100644
> >>>> --- a/src/evdev-mt-touchpad-buttons.c
> >>>> +++ b/src/evdev-mt-touchpad-buttons.c
> >>>> @@ -20,6 +20,8 @@
> >>>> * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> >>>> */
> >>>>
> >>>> +#include "config.h"
> >>>> +
> >>>> #include <errno.h>
> >>>> #include <limits.h>
> >>>> #include <math.h>
> >>>> diff --git a/src/evdev-mt-touchpad-edge-scroll.c
> >>>> b/src/evdev-mt-touchpad-edge-scroll.c
> >>>> index 585c764..2302d2c 100644
> >>>> --- a/src/evdev-mt-touchpad-edge-scroll.c
> >>>> +++ b/src/evdev-mt-touchpad-edge-scroll.c
> >>>> @@ -20,6 +20,8 @@
> >>>> * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> >>>> */
> >>>>
> >>>> +#include "config.h"
> >>>> +
> >>>> #include <errno.h>
> >>>> #include <limits.h>
> >>>> #include <math.h>
> >>>> diff --git a/src/libinput-util.h b/src/libinput-util.h
> >>>> index 732f813..7d5651a 100644
> >>>> --- a/src/libinput-util.h
> >>>> +++ b/src/libinput-util.h
> >>>> @@ -26,6 +26,8 @@
> >>>>
> >>>> #include <unistd.h>
> >>>> #include <math.h>
> >>>> +#include <stdarg.h>
> >>>> +#include <stdio.h>
> >>>> #include <string.h>
> >>>> #include <time.h>
> >>>>
> >>>> @@ -230,6 +232,34 @@ matrix_to_farray6(const struct matrix *m, float
> >>>> out[6])
> >>>> out[5] = m->val[1][2];
> >>>> }
> >>>>
> >>>> +/**
> >>>> + * Simple wrapper for asprintf that ensures the passed in pointer is set
> >>>> + * to NULL upon error.
> >>>> + * The standard asprintf() call does not guarantee the passed in pointer
> >>>> + * will be NULL'ed upon failure, whereas this wrapper does.
> >>>> + *
> >>>> + * @param strp pointer to set to newly allocated string.
> >>>> + * This pointer should be passed to free() to release when done.
> >>>> + * @param fmt the format string to use for printing.
> >>>> + * @return The number of bytes printed (excluding the null byte
> >>>> terminator)
> >>>> + * upon success or -1 upon failure. In the case of failure the pointer is
> >>>> set
> >>>> + * to NULL.
> >>>> + */
> >>>> +static inline int
> >>>> +xasprintf(char **strp, const char *fmt, ...)
> >>>> +{
> >>>> + int rc = 0;
> >>>> + va_list args;
> >>>> +
> >>>> + va_start(args, fmt);
> >>>> + rc = vasprintf(strp, fmt, args);
> >>>> + va_end(args);
> >>>> + if ((rc == -1) && strp)
> >>>> + *strp = NULL;
> >>>> +
> >>>> + return rc;
> >>>> +}
> >>>> +
> >>>> enum ratelimit_state {
> >>>> RATELIMIT_EXCEEDED,
> >>>> RATELIMIT_THRESHOLD,
> >>>> diff --git a/src/timer.c b/src/timer.c
> >>>> index dbd3894..1e507df 100644
> >>>> --- a/src/timer.c
> >>>> +++ b/src/timer.c
> >>>> @@ -20,6 +20,8 @@
> >>>> * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> >>>> */
> >>>>
> >>>> +#include "config.h"
> >>>> +
> >>>> #include <assert.h>
> >>>> #include <errno.h>
> >>>> #include <inttypes.h>
> >>>> diff --git a/test/litest.c b/test/litest.c
> >>>> index 53c441b..b3410bd 100644
> >>>> --- a/test/litest.c
> >>>> +++ b/test/litest.c
> >>>> @@ -876,7 +876,7 @@ litest_init_udev_rules(struct litest_test_device *dev)
> >>>> ck_abort_msg("Failed to create udev rules directory
> >>>> (%s)\n",
> >>>> strerror(errno));
> >>>>
> >>>> - rc = asprintf(&path,
> >>>> + rc = xasprintf(&path,
> >>>> "%s/%s%s.rules",
> >>>> UDEV_RULES_D,
> >>>> UDEV_RULE_PREFIX,
> >>>> diff --git a/tools/libinput-list-devices.c b/tools/libinput-list-devices.c
> >>>> index 6625173..68ddb61 100644
> >>>> --- a/tools/libinput-list-devices.c
> >>>> +++ b/tools/libinput-list-devices.c
> >>>> @@ -120,17 +120,17 @@ calibration_default(struct libinput_device *device)
> >>>> float calibration[6];
> >>>>
> >>>> if (!libinput_device_config_calibration_has_matrix(device)) {
> >>>> - asprintf(&str, "n/a");
> >>>> + xasprintf(&str, "n/a");
> >>>> return str;
> >>>> }
> >>>>
> >>>> if (libinput_device_config_calibration_get_default_matrix(device,
> >>>> calibration) == 0) {
> >>>> - asprintf(&str, "identity matrix");
> >>>> + xasprintf(&str, "identity matrix");
> >>>> return str;
> >>>> }
> >>>>
> >>>> - asprintf(&str,
> >>>> + xasprintf(&str,
> >>>> "%.2f %.2f %.2f %.2f %.2f %.2f",
> >>>> calibration[0],
> >>>> calibration[1],
> >>>> @@ -150,13 +150,13 @@ scroll_defaults(struct libinput_device *device)
> >>>>
> >>>> scroll_methods = libinput_device_config_scroll_get_methods(device);
> >>>> if (scroll_methods == LIBINPUT_CONFIG_SCROLL_NO_SCROLL) {
> >>>> - asprintf(&str, "none");
> >>>> + xasprintf(&str, "none");
> >>>> return str;
> >>>> }
> >>>>
> >>>> method = libinput_device_config_scroll_get_default_method(device);
> >>>>
> >>>> - asprintf(&str,
> >>>> + xasprintf(&str,
> >>>> "%s%s%s%s%s%s",
> >>>> (method == LIBINPUT_CONFIG_SCROLL_2FG) ? "*" : "",
> >>>> (scroll_methods & LIBINPUT_CONFIG_SCROLL_2FG) ?
> >>>> "two-finger " : "",
> >>>> @@ -176,12 +176,12 @@ click_defaults(struct libinput_device *device)
> >>>>
> >>>> click_methods = libinput_device_config_click_get_methods(device);
> >>>> if (click_methods == LIBINPUT_CONFIG_CLICK_METHOD_NONE) {
> >>>> - asprintf(&str, "none");
> >>>> + xasprintf(&str, "none");
> >>>> return str;
> >>>> }
> >>>>
> >>>> method = libinput_device_config_click_get_default_method(device);
> >>>> - asprintf(&str,
> >>>> + xasprintf(&str,
> >>>> "%s%s%s%s",
> >>>> (method == LIBINPUT_CONFIG_CLICK_METHOD_BUTTON_AREAS) ?
> >>>> "*" : "",
> >>>> (click_methods &
> >>>> LIBINPUT_CONFIG_CLICK_METHOD_BUTTON_AREAS) ? "button-areas " : "",
> >>>> --
> >>>> 2.1.0
> >>>>
> >>>> _______________________________________________
> >>>> wayland-devel mailing list
> >>>> wayland-devel at lists.freedesktop.org
> >>>> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> >>>>
> >>>
> >>
> >> --
> >> Jon A. Cruz - Senior Open Source Developer
> >> Samsung Open Source Group
> >> jonc at osg.samsung.com
> >> _______________________________________________
> >> wayland-devel mailing list
> >> wayland-devel at lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> >
> >
> >
>
> --
> Jon A. Cruz - Senior Open Source Developer
> Samsung Open Source Group
> jonc at osg.samsung.com
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
More information about the wayland-devel
mailing list