diff mbox

Avoid overlapping addresses to stpcpy calls in nscd (BZ #16760)

Message ID 20140327192254.GC1982@domone.podge
State Deferred
Headers

Commit Message

Ondrej Bilka March 27, 2014, 7:22 p.m. UTC 
  On Thu, Mar 27, 2014 at 03:34:11AM -0400, Mike Frysinger wrote:
> On Thu 27 Mar 2014 09:34:06 Siddhesh Poyarekar wrote:
> > Calls to stpcpy from nscd netgroups code will have overlapping source
> > and destination when all three values in the returned triplet are
> > non-NULL and in the expected (host,user,domain) order.  This is seen
> > in valgrind as:
> > 
> > Fix this by using memmove instead of stpcpy.  Tested x86_64 using
> > various combinations of triplets (including NULL and non-NULL ones) to
> > verify that this works correctly and there are no regressions.
>
This could work only with additional assertion that we do not move host
forward otherwise it could overwrite user.
 
> i feel like we've wanted an equivalent of stpcpy/memccpy for memmove.  good 
> time to add it ? :)
> 
Yes, it would be better to use this at least internally, perhaps this
patch instead is cleaner. 

Other possibility is keep these in separate header like second snippet, 
do you have better name for that? Also I could make a stpcat and move
equivalent, not sure with what name.

Her I would fix a root cause of these bugs which is bad design. We mix
temporary buffer with building result. If we use separate buffers for
that a code would be lot simpler, I will prepare patch for it.

Comments

Siddhesh Poyarekar March 28, 2014, 2:13 a.m. UTC | #1 
On Thu, Mar 27, 2014 at 08:22:54PM +0100, Ondřej Bílka wrote:
> > > Fix this by using memmove instead of stpcpy.  Tested x86_64 using
> > > various combinations of triplets (including NULL and non-NULL ones) to
> > > verify that this works correctly and there are no regressions.
> >
> This could work only with additional assertion that we do not move host
> forward otherwise it could overwrite user.

If the host, user and domain are out of order, they are copied in
order into a separate area in the buffer before the memmove.  If you
think there's something else that could move host forward then I don't
understand and you'll have to elaborate a bit.

Siddhesh
Ondrej Bilka April 9, 2014, 6:22 p.m. UTC | #2 
On Fri, Mar 28, 2014 at 07:43:22AM +0530, Siddhesh Poyarekar wrote:
> On Thu, Mar 27, 2014 at 08:22:54PM +0100, Ondřej Bílka wrote:
> > > > Fix this by using memmove instead of stpcpy.  Tested x86_64 using
> > > > various combinations of triplets (including NULL and non-NULL ones) to
> > > > verify that this works correctly and there are no regressions.
> > >
> > This could work only with additional assertion that we do not move host
> > forward otherwise it could overwrite user.
> 
> If the host, user and domain are out of order, they are copied in
> order into a separate area in the buffer before the memmove.  If you
> think there's something else that could move host forward then I don't
> understand and you'll have to elaborate a bit.
> 
> Siddhesh

They are in order, its best to show on example what I mean by copying:

host  user  domain
   ^
hoshostser  domain

hoshost tserdomain

hoshost tser domain
Siddhesh Poyarekar April 10, 2014, 12:13 a.m. UTC | #3 
On Wed, Apr 09, 2014 at 08:22:29PM +0200, Ondřej Bílka wrote:
> On Fri, Mar 28, 2014 at 07:43:22AM +0530, Siddhesh Poyarekar wrote:
> > On Thu, Mar 27, 2014 at 08:22:54PM +0100, Ondřej Bílka wrote:
> > > > > Fix this by using memmove instead of stpcpy.  Tested x86_64 using
> > > > > various combinations of triplets (including NULL and non-NULL ones) to
> > > > > verify that this works correctly and there are no regressions.
> > > >
> > > This could work only with additional assertion that we do not move host
> > > forward otherwise it could overwrite user.
> > 
> > If the host, user and domain are out of order, they are copied in
> > order into a separate area in the buffer before the memmove.  If you
> > think there's something else that could move host forward then I don't
> > understand and you'll have to elaborate a bit.
> > 
> > Siddhesh
> 
> They are in order, its best to show on example what I mean by copying:
> 
> host  user  domain
>    ^

OK, now I see what you mean by 'moving host forward'.  We don't do
that.

Siddhesh

> hoshostser  domain
> 
> hoshost tserdomain
> 
> hoshost tser domain
> 
>
diff mbox

Patch

diff --git a/string/extension.h b/string/extension.h
new file mode 100644
index 0000000..963dccf
--- /dev/null
+++ b/string/extension.h
@@ -0,0 +1,36 @@ 
+/* Copyright (C) 2014 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+
+#ifndef	_STRING_EXTENSION_H
+#define	_STRING_EXTENSION_H	1
+
+#include <stdint.h>
+#include <string.h>
+
+
+static inline
+char *
+stpmove (char *dest, const char *src)
+{
+  size_t len = strlen (src);
+  memmove (dest, src, len);
+  return dest + len;
+}
+
+
+#endif