Follow-up Comment #4, bug #67899 (group groff): The code still isn't as tidy with memory as it could be.
Instrumenting it:
$ git diff
diff --git a/src/preproc/pic/lex.cpp b/src/preproc/pic/lex.cpp
index 0cf109bd4..c8f861484 100644
--- a/src/preproc/pic/lex.cpp
+++ b/src/preproc/pic/lex.cpp
@@ -399,6 +399,7 @@ void interpolate_macro_with_args(const char *body)
int level = 0;
int c;
enum { NORMAL, IN_STRING, IN_STRING_QUOTED } state = NORMAL;
+ debug("GBR: char *argv[%1] declared", MAX_ARG);
do {
token_buffer.clear();
for (;;) {
@@ -417,6 +418,7 @@ void interpolate_macro_with_args(const char *body)
}
if (token_buffer.length() > 0) {
token_buffer += '\0';
+ debug("GBR: populating argv[%1]", argc);
argv[argc] = strsave(token_buffer.contents());
}
// for 'foo()', argc = 0
And feeding it the (slightly cosmetically cleaned-up) reproducer, we see a
lingering issue.
$ cat ./ATTIC/67899-crasher.roff
.PS
# Plot a single jumper in a $1 by $2 box, $3 is the on-off state
define jumper { [
shrinkfactor = 0.8;
Outer: box invis wid 0.5 ht 1;
# Count on end ] to reset these
boxwid = Outer.wid * shrinkfactor / 2;
boxht = Outer.ht * shrinkfactor / 2;
box fill (!$1) with .s at center of Outer;
box fill ($1) with .n at center of Outer;
] }
# Plot a block of six jumpers
define jumperblock {
jumper($1);
jumper($2);
jumper($3);
jumper($4);
jumper($5);
jumper($6);
jwidth = last [].Outer.wid;
jheight = last [].Outer.ht;
box with .nw at 6th last [].nw wid 6*jwidth ht jheight;
# Use {} to avoid changing position from last box draw.
# This is necessary so move in any direction will work as expected
{"Jumpers in state $1$2$2$3$4$5$6" at last box .s + (0, -0.2);}
}
# Sample macro invocations
jumperblock(1,1,0,0,1,0);
move;
jumperblock(0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,\
23,24,25,26,27,28,29,30,31,,0AAAA,1AAAB,,,,,,7AAAH,,9AAAJA,\
10AAAK,11AAAL,12AAAM,13AAAN,14AAAO,15AAAP,16AAAQ,17AAAR,\
18AAAS,19AAAT,20AAAU,21AAAAAAV);
.PE
$ ./build/pic ./ATTIC/67899-crasher.roff
.do if !dPS .ds PS
.do if !dPE .ds PE
.do if !dPF .ds PF
.do if !dPY .ds PY
.lf 0 "./ATTIC/67899-crasher.roff
.lf 1
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: char *argv[32]
declared
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: populating argv[0]
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: populating argv[1]
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: populating argv[2]
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: populating argv[3]
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: populating argv[4]
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: populating argv[5]
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: char *argv[32]
declared
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: populating argv[0]
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: char *argv[32]
declared
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: populating argv[0]
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: char *argv[32]
declared
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: populating argv[0]
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: char *argv[32]
declared
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: populating argv[0]
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: char *argv[32]
declared
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: populating argv[0]
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: char *argv[32]
declared
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: populating argv[0]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: char *argv[32]
declared
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[0]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[1]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[2]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[3]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[4]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[5]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[6]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[7]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[8]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[9]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[10]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[11]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[12]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[13]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[14]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[15]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[16]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[17]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[18]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[19]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[20]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[21]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[22]
./build/pic:./ATTIC/67899-crasher.roff:37: debug: GBR: populating argv[23]
./build/pic:./ATTIC/67899-crasher.roff:37: debug: GBR: populating argv[24]
./build/pic:./ATTIC/67899-crasher.roff:37: debug: GBR: populating argv[25]
./build/pic:./ATTIC/67899-crasher.roff:37: debug: GBR: populating argv[26]
./build/pic:./ATTIC/67899-crasher.roff:37: debug: GBR: populating argv[27]
./build/pic:./ATTIC/67899-crasher.roff:37: debug: GBR: populating argv[28]
./build/pic:./ATTIC/67899-crasher.roff:37: debug: GBR: populating argv[29]
./build/pic:./ATTIC/67899-crasher.roff:37: debug: GBR: populating argv[30]
./build/pic:./ATTIC/67899-crasher.roff:37: debug: GBR: populating argv[31]
./build/pic:./ATTIC/67899-crasher.roff:37: warning: pic supports at most 32
macro arguments
./build/pic:./ATTIC/67899-crasher.roff:37: debug: GBR: populating argv[32]
./build/pic:./ATTIC/67899-crasher.roff:37: debug: GBR: populating argv[32]
./build/pic:./ATTIC/67899-crasher.roff:37: debug: GBR: populating argv[32]
./build/pic:./ATTIC/67899-crasher.roff:37: debug: GBR: populating argv[32]
./build/pic:./ATTIC/67899-crasher.roff:38: debug: GBR: populating argv[32]
./build/pic:./ATTIC/67899-crasher.roff:38: debug: GBR: populating argv[32]
./build/pic:./ATTIC/67899-crasher.roff:38: debug: GBR: populating argv[32]
./build/pic:./ATTIC/67899-crasher.roff:38: debug: GBR: populating argv[32]
./build/pic:./ATTIC/67899-crasher.roff:38: debug: GBR: populating argv[32]
./build/pic:./ATTIC/67899-crasher.roff:38: debug: GBR: populating argv[32]
./build/pic:./ATTIC/67899-crasher.roff:38: debug: GBR: populating argv[32]
./build/pic:./ATTIC/67899-crasher.roff:38: debug: GBR: populating argv[32]
./build/pic:./ATTIC/67899-crasher.roff:39: debug: GBR: populating argv[32]
./build/pic:./ATTIC/67899-crasher.roff:39: debug: GBR: populating argv[32]
./build/pic:./ATTIC/67899-crasher.roff:39: debug: GBR: populating argv[32]
./build/pic:./ATTIC/67899-crasher.roff:39: debug: GBR: populating argv[32]
./build/pic:./ATTIC/67899-crasher.roff:39: debug: GBR: char *argv[32]
declared
[...snip...]
We are writing a pointer one past the end of our argv array, and overwriting
it repeatedly.
This is not anywhere near as bad as the original problem but it's still not
good.
Now the problem is that we're not testing the `ignore` variable when
encountering *non*-empty arguments.
(The string contents themselves are stored on the heap via objects of
_groff_'s "string" class--not the
C++ one. For the purposes of this defect report, all we are using the stack
for is the local `char *argv[]`
array. No matter what the sizes of the macro arguments themselves are, all
we're writing to the *stack*
are pointers.)
A further slight code reorganization:
$ git stash show -p 0
diff --git a/src/preproc/pic/lex.cpp b/src/preproc/pic/lex.cpp
index 0cf109bd4..bc62cb4be 100644
--- a/src/preproc/pic/lex.cpp
+++ b/src/preproc/pic/lex.cpp
@@ -414,10 +414,10 @@ void interpolate_macro_with_args(const char *body)
MAX_ARG);
ignore = 1;
}
- }
- if (token_buffer.length() > 0) {
- token_buffer += '\0';
- argv[argc] = strsave(token_buffer.contents());
+ else if (token_buffer.length() > 0) {
+ token_buffer += '\0';
+ argv[argc] = strsave(token_buffer.contents());
+ }
}
// for 'foo()', argc = 0
if (argc > 0 || c != ')' || i > 0)
...clears that up.
$ ./build/pic ./ATTIC/67899-crasher.roff
.do if !dPS .ds PS
.do if !dPE .ds PE
.do if !dPF .ds PF
.do if !dPY .ds PY
.lf 0 "./ATTIC/67899-crasher.roff
.lf 1
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: char *argv[32]
declared
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: populating argv[0]
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: populating argv[1]
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: populating argv[2]
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: populating argv[3]
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: populating argv[4]
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: populating argv[5]
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: char *argv[32]
declared
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: populating argv[0]
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: char *argv[32]
declared
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: populating argv[0]
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: char *argv[32]
declared
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: populating argv[0]
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: char *argv[32]
declared
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: populating argv[0]
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: char *argv[32]
declared
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: populating argv[0]
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: char *argv[32]
declared
./build/pic:./ATTIC/67899-crasher.roff:34: debug: GBR: populating argv[0]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: char *argv[32]
declared
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[0]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[1]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[2]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[3]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[4]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[5]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[6]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[7]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[8]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[9]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[10]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[11]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[12]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[13]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[14]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[15]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[16]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[17]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[18]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[19]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[20]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[21]
./build/pic:./ATTIC/67899-crasher.roff:36: debug: GBR: populating argv[22]
./build/pic:./ATTIC/67899-crasher.roff:37: debug: GBR: populating argv[23]
./build/pic:./ATTIC/67899-crasher.roff:37: debug: GBR: populating argv[24]
./build/pic:./ATTIC/67899-crasher.roff:37: debug: GBR: populating argv[25]
./build/pic:./ATTIC/67899-crasher.roff:37: debug: GBR: populating argv[26]
./build/pic:./ATTIC/67899-crasher.roff:37: debug: GBR: populating argv[27]
./build/pic:./ATTIC/67899-crasher.roff:37: debug: GBR: populating argv[28]
./build/pic:./ATTIC/67899-crasher.roff:37: debug: GBR: populating argv[29]
./build/pic:./ATTIC/67899-crasher.roff:37: debug: GBR: populating argv[30]
./build/pic:./ATTIC/67899-crasher.roff:37: debug: GBR: populating argv[31]
./build/pic:./ATTIC/67899-crasher.roff:37: warning: pic supports at most 32
macro arguments
./build/pic:./ATTIC/67899-crasher.roff:39: debug: GBR: char *argv[32]
declared
./build/pic:./ATTIC/67899-crasher.roff:39: debug: GBR: populating argv[0]
[...snip...]
Oh, and hey, look--the line numbers in the diagnostics are off when doing
nested macro interpolations.
Not even gonna deal with that for this ticket or even this release. The
hobnailed boots of progress are on the march!
_______________________________________________________
Reply to this item at:
<https://savannah.gnu.org/bugs/?67899>
_______________________________________________
Message sent via Savannah
https://savannah.gnu.org/
signature.asc
Description: PGP signature
