[0/5] obstacks again
diff mbox

Message ID 5450983F.3030608@cs.ucla.edu
State New
Headers show

Commit Message

Paul Eggert Oct. 29, 2014, 7:33 a.m. UTC
Thanks for doing all this.  The gnulib patches are good as far as they go, but 
they need one more change: alignments should also change from int to size_t. 
The first attached gnulib patch does that, plus it fixes a longstanding integer 
overflow bug that can occur with large alignments (plus large sizes).  While 
we're in the neighborhood we should be using C11's alignof rather than 
reinventing that particular wheel; the second attached gnulib patch does that. 
I've installed your five gnulib patches plus the two attached patches into gnulib.

Two things for the glibc patch.  First, the updated gnulib patches need to be 
merged into the glibc patch.  Second, the manual needs to be updated to match 
the revised API induced by all these patches.

And thanks again.
From 421a53ad006c9ad9a1cad0b1e42246c01ffa3154 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 28 Oct 2014 23:58:42 -0700
Subject: [PATCH 1/2] obstack: use size_t alignments and check for overflow

* lib/obstack.c, lib/obstack.h (_obstack_begin, _obstack_begin_1):
* lib/obstack.c (_obstack_begin_worker, _obstack_newchunk):
* lib/obstack.h (struct obstack.alignment_mask):
Use _OBSTACK_SIZE_T, not int, for alignments.
* lib/obstack.c (_obstack_newchunk): Fail if the size calculation
overflows, e.g., when adding the alignment.
---
 ChangeLog     | 10 ++++++++++
 lib/obstack.c | 18 +++++++++++-------
 lib/obstack.h |  8 +++++---
 3 files changed, 26 insertions(+), 10 deletions(-)

Comments

Joseph Myers Oct. 29, 2014, 6:31 p.m. UTC | #1
On Wed, 29 Oct 2014, Paul Eggert wrote:

> While we're in the neighborhood we should be using C11's alignof rather than
> reinventing that particular wheel; the second attached gnulib patch does that.

Note that we can't use <stdalign.h> in glibc sources unless we move to 
requiring GCC >= 4.7 (and it's not clear whether we'd get consensus for 
such a requirement if proposed).
Alan Modra Oct. 29, 2014, 10:02 p.m. UTC | #2
On Wed, Oct 29, 2014 at 12:33:19AM -0700, Paul Eggert wrote:
> Thanks for doing all this.  The gnulib patches are good as far as they go,
> but they need one more change: alignments should also change from int to
> size_t. The first attached gnulib patch does that, plus it fixes a
> longstanding integer overflow bug that can occur with large alignments (plus
> large sizes).  While we're in the neighborhood we should be using C11's
> alignof rather than reinventing that particular wheel; the second attached
> gnulib patch does that. I've installed your five gnulib patches plus the two
> attached patches into gnulib.

One thing though, I didn't put the ChangeLog diffs in the patch as I
usually add them when committing.

> Two things for the glibc patch.  First, the updated gnulib patches need to
> be merged into the glibc patch.  Second, the manual needs to be updated to
> match the revised API induced by all these patches.

Ow!  The manual reflects obstacks as they were 20 years ago.

Hmm, looking over it I see a misfeature that I've removed.

@cindex shrinking objects
You can use @code{obstack_blank} with a negative size argument to make
the current object smaller.  Just don't try to shrink it beyond zero
length---there's no telling what will happen if you do that.

It is no longer possible to shrink an obstack with obstack_blank (but
you can still do that with obstack_blank_fast).
Paul Eggert Oct. 30, 2014, 3:35 a.m. UTC | #3
Alan Modra wrote:

> One thing though, I didn't put the ChangeLog diffs in the patch as I
> usually add them when committing.

Oh, I missed that.  I added them now.  For Gnulib it's better to put them into 
the patch.

> It is no longer possible to shrink an obstack with obstack_blank (but
> you can still do that with obstack_blank_fast).

Ouch, I hadn't noticed that.  That's an incompatible change and I expect it will 
break real-world usage for no particularly good reason, so we really need to fix 
this.  How about making the 2nd argument to obstack_blank and obstack_blank_fast 
be of type ptrdiff_t rather than size_t?
Alan Modra Oct. 30, 2014, 6:07 a.m. UTC | #4
On Wed, Oct 29, 2014 at 08:35:18PM -0700, Paul Eggert wrote:
> Alan Modra wrote:
> >It is no longer possible to shrink an obstack with obstack_blank (but
> >you can still do that with obstack_blank_fast).
> 
> Ouch, I hadn't noticed that.  That's an incompatible change and I expect it
> will break real-world usage for no particularly good reason, so we really
> need to fix this.  How about making the 2nd argument to obstack_blank and
> obstack_blank_fast be of type ptrdiff_t rather than size_t?

We could, but..  I expect there is going to be other downstream code
breakage anyway.  Witness the patches needed to make gcc work with a
more modern obstack.h, where a seemingly innocuous change like casting
the obstack_base value to void* broke code doing pointer arithmetic,
and gcc's use of the internal _obstack_begin function meant that gcc
was exposed to the changed chunkfun type.

Now if the 2nd arg of obstack_blank changes to type ptrdiff_t, then it
becomes an odd case, all the other obstack "functions" taking a size_t.
Also, I'd say obstack_blank is the wrong macro to use for shrinking,
the most natural one being obstack_blank_fast due to it bypassing the
obstack_room check.

So there are my reasons for leaving obstack_blank as is.  However, if
you and others prefer to go with ptrdiff_t I'm happy enough to submit
a patch.

Oh, and while I'm advocating breakage, the doc for obstack_next_free
says it returns a void* but actually returns a char*.  Since it was
deemed a good idea to similarly correct obstack_base, should we do the
same for obstack_next_free?
Eric Blake Dec. 5, 2014, 7:13 p.m. UTC | #5
On 10/29/2014 09:35 PM, Paul Eggert wrote:
> Alan Modra wrote:
> 
>> One thing though, I didn't put the ChangeLog diffs in the patch as I
>> usually add them when committing.
> 
> Oh, I missed that.  I added them now.  For Gnulib it's better to put
> them into the patch.
> 
>> It is no longer possible to shrink an obstack with obstack_blank (but
>> you can still do that with obstack_blank_fast).
> 
> Ouch, I hadn't noticed that.  That's an incompatible change and I expect
> it will break real-world usage for no particularly good reason, so we
> really need to fix this.  How about making the 2nd argument to
> obstack_blank and obstack_blank_fast be of type ptrdiff_t rather than
> size_t?

It breaks GNU M4, for a starter :)  But at least we predicted that it
would happen, and I'm hoping the fallback of obstack_blank_fast does the
job.
Gary V. Vaughan Dec. 5, 2014, 9:02 p.m. UTC | #6
On Dec 5, 2014, at 7:13 PM, Eric Blake <eblake@redhat.com> wrote:
> On 10/29/2014 09:35 PM, Paul Eggert wrote:
>> Alan Modra wrote:
>> 
>>> One thing though, I didn't put the ChangeLog diffs in the patch as I
>>> usually add them when committing.
>> 
>> Oh, I missed that.  I added them now.  For Gnulib it's better to put
>> them into the patch.
>> 
>>> It is no longer possible to shrink an obstack with obstack_blank (but
>>> you can still do that with obstack_blank_fast).
>> 
>> Ouch, I hadn't noticed that.  That's an incompatible change and I expect
>> it will break real-world usage for no particularly good reason, so we
>> really need to fix this.  How about making the 2nd argument to
>> obstack_blank and obstack_blank_fast be of type ptrdiff_t rather than
>> size_t?
> 
> It breaks GNU M4, for a starter :)  But at least we predicted that it
> would happen, and I'm hoping the fallback of obstack_blank_fast does the
> job.

It does, thanks.

Although for someone not following bug-gnulib fairly carefully, it is far
from obvious why bumping gnulib makes your application suddenly use up all
the available memory.  Which is a shame, because, other than that M4 would
have been perfectly functional after the gnulib bump without any special
client code changes.

If there's a way to at least diagnose negative arguments rather than silently
change behavior, that would save other projects some migration headaches...

Cheers,
Alan Modra Dec. 6, 2014, 1:49 a.m. UTC | #7
On Fri, Dec 05, 2014 at 09:02:46PM +0000, Gary V. Vaughan wrote:
> If there's a way to at least diagnose negative arguments rather than silently
> change behavior, that would save other projects some migration headaches...

We could diagnose one of the m4 uses of obstack_blank at compile time,
but not the one in m4/macro.c:trace_flush which has a non-constant
shrink value..

Yes, I agree this can cause some pain, but at least dying with
out-of-memory is a loud symptom.

The alternative is to do as Paul suggested and make obstack_blank
accept a negative length argument, but that would
- kill >2G obstacks on 32-bit targets,
- lose the nice symmetry with other obstack functions,
and obstack_blank_fast is the right interface to use for shrinking.
Andreas Schwab Dec. 6, 2014, 9:48 a.m. UTC | #8
Alan Modra <amodra@gmail.com> writes:

> The alternative is to do as Paul suggested and make obstack_blank
> accept a negative length argument, but that would
> - kill >2G obstacks on 32-bit targets,
> - lose the nice symmetry with other obstack functions,
> and obstack_blank_fast is the right interface to use for shrinking.

It would reintroduce the nice symmetry that the *_fast functions are
just optimisations of the non-fast functions (if you know that the
obstack has enough space), without surprising behaviour change.

Andreas.

Patch
diff mbox

diff --git a/ChangeLog b/ChangeLog
index 7aac7eb..8bf2baa 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@ 
+2014-10-28  Paul Eggert  <eggert@cs.ucla.edu>
+
+	obstack: use size_t alignments and check for overflow
+	* lib/obstack.c, lib/obstack.h (_obstack_begin, _obstack_begin_1):
+	* lib/obstack.c (_obstack_begin_worker, _obstack_newchunk):
+	* lib/obstack.h (struct obstack.alignment_mask):
+	Use _OBSTACK_SIZE_T, not int, for alignments.
+	* lib/obstack.c (_obstack_newchunk): Fail if the size calculation
+	overflows, e.g., when adding the alignment.
+
 2014-10-24  Paul Eggert  <eggert@cs.ucla.edu>
 
 	socketlib, sockets, sys_socket: Use AC_REQUIRE to pacify autoconf.
diff --git a/lib/obstack.c b/lib/obstack.c
index 8e247fb..342f9f8 100644
--- a/lib/obstack.c
+++ b/lib/obstack.c
@@ -106,7 +106,7 @@  typedef void (*freefun_type) (void *, struct _obstack_chunk *);
 
 static int
 _obstack_begin_worker (struct obstack *h,
-                       _OBSTACK_SIZE_T size, int alignment,
+                       _OBSTACK_SIZE_T size, _OBSTACK_SIZE_T alignment,
                        chunkfun_type chunkfun, freefun_type freefun)
 {
   struct _obstack_chunk *chunk; /* points to new chunk */
@@ -150,7 +150,7 @@  _obstack_begin_worker (struct obstack *h,
 
 int
 _obstack_begin (struct obstack *h,
-                _OBSTACK_SIZE_T size, int alignment,
+                _OBSTACK_SIZE_T size, _OBSTACK_SIZE_T alignment,
                 void *(*chunkfun) (size_t),
                 void (*freefun) (void *))
 {
@@ -162,7 +162,7 @@  _obstack_begin (struct obstack *h,
 
 int
 _obstack_begin_1 (struct obstack *h,
-                  _OBSTACK_SIZE_T size, int alignment,
+                  _OBSTACK_SIZE_T size, _OBSTACK_SIZE_T alignment,
                   void *(*chunkfun) (void *, size_t),
                   void (*freefun) (void *, void *),
                   void *arg)
@@ -184,18 +184,22 @@  void
 _obstack_newchunk (struct obstack *h, _OBSTACK_SIZE_T length)
 {
   struct _obstack_chunk *old_chunk = h->chunk;
-  struct _obstack_chunk *new_chunk;
-  size_t new_size;
+  struct _obstack_chunk *new_chunk = 0;
   size_t obj_size = h->next_free - h->object_base;
   char *object_base;
 
   /* Compute size for new chunk.  */
-  new_size = (obj_size + length) + (obj_size >> 3) + h->alignment_mask + 100;
+  size_t sum1 = obj_size + length;
+  size_t sum2 = sum1 + h->alignment_mask;
+  size_t new_size = sum2 + (obj_size >> 3) + 100;
+  if (new_size < sum2)
+    new_size = sum2;
   if (new_size < h->chunk_size)
     new_size = h->chunk_size;
 
   /* Allocate and initialize the new chunk.  */
-  new_chunk = CALL_CHUNKFUN (h, new_size);
+  if (obj_size <= sum1 && sum1 <= sum2)
+    new_chunk = CALL_CHUNKFUN (h, new_size);
   if (!new_chunk)
     (*obstack_alloc_failed_handler)();
   h->chunk = new_chunk;
diff --git a/lib/obstack.h b/lib/obstack.h
index 909d0d3..ba4de1d 100644
--- a/lib/obstack.h
+++ b/lib/obstack.h
@@ -166,7 +166,7 @@  struct obstack          /* control current object in current chunk */
     _OBSTACK_SIZE_T i;
     void *p;
   } temp;                       /* Temporary for some macros.  */
-  int alignment_mask;           /* Mask of alignment for each object. */
+  _OBSTACK_SIZE_T alignment_mask;  /* Mask of alignment for each object. */
   /* These prototypes vary based on 'use_extra_arg', and we use
      casts to the prototypeless function type in all assignments,
      but having prototypes here quiets -Wstrict-prototypes.  */
@@ -187,9 +187,11 @@  struct obstack          /* control current object in current chunk */
 
 extern void _obstack_newchunk (struct obstack *, _OBSTACK_SIZE_T);
 extern void _obstack_free (struct obstack *, void *);
-extern int _obstack_begin (struct obstack *, _OBSTACK_SIZE_T, int,
+extern int _obstack_begin (struct obstack *,
+                           _OBSTACK_SIZE_T, _OBSTACK_SIZE_T,
                            void *(*)(size_t), void (*)(void *));
-extern int _obstack_begin_1 (struct obstack *, _OBSTACK_SIZE_T, int,
+extern int _obstack_begin_1 (struct obstack *,
+                             _OBSTACK_SIZE_T, _OBSTACK_SIZE_T,
                              void *(*)(void *, size_t),
                              void (*)(void *, void *), void *);
 extern _OBSTACK_SIZE_T _obstack_memory_used (struct obstack *)