[Slirp] [PATCH 1/6] util: add slirp_fmt() helpers
Marc-André Lureau
marcandre.lureau at redhat.com
Mon Jan 27 09:58:33 UTC 2020
Hi
On Mon, Jan 27, 2020 at 10:43 AM Philippe Mathieu-Daudé
<philmd at redhat.com> wrote:
>
> On 1/27/20 10:24 AM, marcandre.lureau at redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau at redhat.com>
> >
> > Various calls to snprintf() in libslirp assume that snprintf() returns
> > "only" the number of bytes written (excluding terminating NUL).
> >
> > https://pubs.opengroup.org/onlinepubs/9699919799/functions/snprintf.html#tag_16_159_04
> >
> > "Upon successful completion, the snprintf() function shall return the
> > number of bytes that would be written to s had n been sufficiently
> > large excluding the terminating null byte."
> >
> > Introduce slirp_fmt() that handles several pathological cases the
> > way libslirp usually expect:
> >
> > - treat error as fatal (instead of silently returning -1)
> >
> > - fmt0() will always \0 end
> >
> > - return the number of bytes actually written (instead of what would
> > have been written, which would usually result in OOB later), including
> > the ending \0 for fmt0()
>
>
> Arf I started the same work... But only slirp_fmt(), no slirp_fmt0() so
> far. Good you already finished it.
Thanks for the review. I started working on it last week, and opened a
confidential issue in gitlab. Sadly, it is not very flexible (can't
add non-admin people, private MR kinda broken).
> > - warn if truncation happened (instead of ignoring)
> >
> > Other less common cases can still be handled with strcpy/snprintf() etc.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
> > Reviewed-by: Samuel Thibault <samuel.thibault at ens-lyon.org>
>
> In my patches I used:
> Suggested-by: Laszlo Ersek <lersek at redhat.com>
>
> > ---
> > src/util.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > src/util.h | 3 +++
> > 2 files changed, 65 insertions(+)
> >
> > diff --git a/src/util.c b/src/util.c
> > index e596087..e3b6257 100644
> > --- a/src/util.c
> > +++ b/src/util.c
> > @@ -364,3 +364,65 @@ void slirp_pstrcpy(char *buf, int buf_size, const char *str)
> > }
> > *q = '\0';
> > }
> > +
> > +static int slirp_vsnprintf(char *str, size_t size,
> > + const char *format, va_list args)
> > +{
> > + int rv = vsnprintf(str, size, format, args);
> > +
> > + if (rv < 0) {
> > + g_error("vsnprintf() failed: %s", g_strerror(errno));
>
> So here we abort() the program.
> This is "dies on error".
>
> > + }
> > +
> > + return rv;
> > +}
> > +
> > +/*
> > + * A snprintf()-like function that:
> > + * - returns the number of bytes written (excluding optional \0-ending)
> > + * - dies on error
> > + * - warn on truncation
>
> "warns"
>
> > + */
> > +int slirp_fmt(char *str, size_t size, const char *format, ...)
> > +{
> > + va_list args;
> > + int rv;
> > +
> > + va_start(args, format);
> > + rv = slirp_vsnprintf(str, size, format, args);
> > + va_end(args);
> > +
> > + if (rv > size) {
> > + g_critical("vsnprintf() truncation");
>
> Here we don't abort.
> This is "warn on truncation".
>
> IIRC Daniel or David raised the problem of using GLib functions that can
> be disabled at compile-time.
Can be disabled? There is:
- G_DISABLE_ASSERT, which removes g_assert*() code
- G_DISABLE_CHECKS, which will remove g_return*() code.
g_error() uses G_LOG_LEVEL_ERROR which is always fatal.
So, there shouldn't be a problem here.
>
> > + }
> > +
> > + return MIN(rv, size);
> > +}
> > +
> > +/*
> > + * A snprintf()-like function that:
> > + * - always \0-end (unless size == 0)
> > + * - returns the number of bytes actually written, including \0 ending
> > + * - dies on error
> > + * - warn on truncation
>
> "warns"
>
> > + */
> > +int slirp_fmt0(char *str, size_t size, const char *format, ...)
> > +{
> > + va_list args;
> > + int rv;
> > +
> > + va_start(args, format);
> > + rv = slirp_vsnprintf(str, size, format, args);
> > + va_end(args);
> > +
> > + if (rv >= size) {
> > + g_critical("vsnprintf() truncation");
> > + if (size > 0)
> > + str[size - 1] = '\0';
> > + rv = size;
> > + } else {
> > + rv += 1; /* include \0 */
> > + }
> > +
> > + return rv;
> > +}
> > diff --git a/src/util.h b/src/util.h
> > index e9c3073..5530c46 100644
> > --- a/src/util.h
> > +++ b/src/util.h
> > @@ -181,4 +181,7 @@ static inline int slirp_socket_set_fast_reuse(int fd)
> >
> > void slirp_pstrcpy(char *buf, int buf_size, const char *str);
> >
> > +int slirp_fmt(char *str, size_t size, const char *format, ...);
> > +int slirp_fmt0(char *str, size_t size, const char *format, ...);
> > +
> > #endif
> >
>
More information about the Slirp
mailing list