* Re: [Emacs-diffs] /srv/bzr/emacs/trunk r105581: Integer and memory overflow issues (Bug#9196).
2011-08-27 3:28 ` [Emacs-diffs] /srv/bzr/emacs/trunk r105581: Integer and memory overflow issues (Bug#9196) Stefan Monnier
@ 2011-08-27 7:17 ` Paul Eggert
2011-08-27 7:17 ` bug#9196: " Paul Eggert
1 sibling, 0 replies; 3+ messages in thread
From: Paul Eggert @ 2011-08-27 7:17 UTC (permalink / raw
To: Stefan Monnier; +Cc: 9196, emacs-devel
In <http://lists.gnu.org/archive/html/emacs-devel/2011-08/msg00979.html>
Stefan Monnier wrote:
> I don't want to sprinkle memory_full calls all over the code like your
> patch does.
I'd rather avoid them too. That is, I'd like to revamp the code to
avoid the need for most of the recently-introduced memory_full calls.
(Counting the further patch discussed below, 11 memory_full calls were
recently removed and 27 added, for a net gain of 16.) However, I
don't see an easy way to do that. Perhaps we can revisit this after
Emacs 24 is out, when it'll be possible to do some more-serious code
reorganization.
I looked again at the part of the patch that you quoted, and it turned
out to be overly conservative: it introduced an integer overflow check
that wasn't needed, regardless of whether --with-wide-int is used.
(This is because CCL_EXECUTE_BUF_SIZE is less than INT_MAX / 5; I
hadn't noticed that earlier, and thanks for catching that problem.)
I just now removed that overflow check from the trunk, as part of bzr
105585.
> many of the overflow risks it fixes can only happen in
> the case where we use 64bit Lisp_Objects on a 32bit system,
Sorry, I don't follow this remark. As far as I know, the
recently-introduced overflow checks are needed even if configure's
--with-wide-int option is not used.
> right now, those overflows don't worry me as much as the code
> cleanliness which seems to suffer from your patch.
Thanks to your comments, src/ccl.c is now shorter than it was before
that patch was installed, so that part of the change should be OK now.
I'll be happy to try to address any other parts of the change that
need similar improvements. (It is a 6000-line patch, after all, and a
patch that large can always be improved....) That being said, I'm
afraid that Emacs's buffer and integer overflow bugs cannot be fixed
without adding *some* complications -- unless we're willing to do a
pretty drastic rewrite of the internals.
>> > +enum
>> > + {
>> > + bidi_shelve_header_size =
>> > + (sizeof (bidi_cache_idx) + sizeof (bidi_cache_start_stack)
>> > + + sizeof (bidi_cache_sp) + sizeof (bidi_cache_start)
>> > + + sizeof (bidi_cache_last_idx))
>> > + };
> Why an enum? Sounds really ugly!
This was to avoid duplication of that long expression. The
duplication was already present in the old version, but the patch
duplicated the expression one more time, which was a minor annoyance.
If you prefer the old way of doing things, where the expression is
duplicated, I can easily get rid of bidi_shelve_header_size entirely.
Or, if you prefer a #define to the enum, I can easily change the code
to do that. Enums often work better with debuggers, though.
^ permalink raw reply [flat|nested] 3+ messages in thread* bug#9196: [Emacs-diffs] /srv/bzr/emacs/trunk r105581: Integer and memory overflow issues (Bug#9196).
2011-08-27 3:28 ` [Emacs-diffs] /srv/bzr/emacs/trunk r105581: Integer and memory overflow issues (Bug#9196) Stefan Monnier
2011-08-27 7:17 ` Paul Eggert
@ 2011-08-27 7:17 ` Paul Eggert
1 sibling, 0 replies; 3+ messages in thread
From: Paul Eggert @ 2011-08-27 7:17 UTC (permalink / raw
To: Stefan Monnier; +Cc: 9196, emacs-devel
In <http://lists.gnu.org/archive/html/emacs-devel/2011-08/msg00979.html>
Stefan Monnier wrote:
> I don't want to sprinkle memory_full calls all over the code like your
> patch does.
I'd rather avoid them too. That is, I'd like to revamp the code to
avoid the need for most of the recently-introduced memory_full calls.
(Counting the further patch discussed below, 11 memory_full calls were
recently removed and 27 added, for a net gain of 16.) However, I
don't see an easy way to do that. Perhaps we can revisit this after
Emacs 24 is out, when it'll be possible to do some more-serious code
reorganization.
I looked again at the part of the patch that you quoted, and it turned
out to be overly conservative: it introduced an integer overflow check
that wasn't needed, regardless of whether --with-wide-int is used.
(This is because CCL_EXECUTE_BUF_SIZE is less than INT_MAX / 5; I
hadn't noticed that earlier, and thanks for catching that problem.)
I just now removed that overflow check from the trunk, as part of bzr
105585.
> many of the overflow risks it fixes can only happen in
> the case where we use 64bit Lisp_Objects on a 32bit system,
Sorry, I don't follow this remark. As far as I know, the
recently-introduced overflow checks are needed even if configure's
--with-wide-int option is not used.
> right now, those overflows don't worry me as much as the code
> cleanliness which seems to suffer from your patch.
Thanks to your comments, src/ccl.c is now shorter than it was before
that patch was installed, so that part of the change should be OK now.
I'll be happy to try to address any other parts of the change that
need similar improvements. (It is a 6000-line patch, after all, and a
patch that large can always be improved....) That being said, I'm
afraid that Emacs's buffer and integer overflow bugs cannot be fixed
without adding *some* complications -- unless we're willing to do a
pretty drastic rewrite of the internals.
>> > +enum
>> > + {
>> > + bidi_shelve_header_size =
>> > + (sizeof (bidi_cache_idx) + sizeof (bidi_cache_start_stack)
>> > + + sizeof (bidi_cache_sp) + sizeof (bidi_cache_start)
>> > + + sizeof (bidi_cache_last_idx))
>> > + };
> Why an enum? Sounds really ugly!
This was to avoid duplication of that long expression. The
duplication was already present in the old version, but the patch
duplicated the expression one more time, which was a minor annoyance.
If you prefer the old way of doing things, where the expression is
duplicated, I can easily get rid of bidi_shelve_header_size entirely.
Or, if you prefer a #define to the enum, I can easily change the code
to do that. Enums often work better with debuggers, though.
^ permalink raw reply [flat|nested] 3+ messages in thread