prevent memory overflow in compilePassOpcode

[mgieseki]

This is a proposal to fix issue #1171. To prevent writing past the CharsString memory block, I replaced all assignments to passInstructions[passIC++] with macro APPEND_INSTRUCTION_CHAR(ch) that checks the validity of index passIC before assigning a value.
The sample file used to trigger the buffer overflow in #1171 only affects pass_lookback. But since there are a couple of similar sections that append more than one character without checking the index, I adapted all of them. The "ugly" macro could be replaced by dedicated index checks to reduce the number of if statements and to speed up the code a bit, but I decided against that to avoid code bloat.

[bertfrees]

Thanks! But why do we need a macro? Couldn't it be done with a function?

If we do need a macro I suggest we wrap it in do {...} while (0).

[mgieseki]

I think we need a macro if we want to leave function compilePassOpcode once the maximum string length is reached. Otherwise, another check containing a return statement must be added inside compilePassOpcode.

Maybe it's not necessary to check the index for each single character and we can avoid to introduce the macro. If I counted correctly, there's a maximum number of 7 potentially untested assignments to passInstructions[passIC++] per loop iteration. So it would be sufficient to ensure that there's enough space to hold 7 additional characters at the beginning of the loop body. I.e. replacing line

liblouis/liblouis/compileTranslationTable.c

Line 1886 in f2251cb

if (passIC >= MAXSTRING) {

with

if (passIC >= MAXSTRING-6) {

should also fix the issue, but I haven't tested it properly yet.

[bertfrees]

Like this?

if (!append_instruction_char(passInstructions, &passIC, pass_lookback)) return 0;

I prefer that over a macro.

if (passIC >= MAXSTRING-6)

I could live with this solution too, if it's explained what the "6" is, and in such a way that it's obvious the number needs to be updated when we modify compilePassOpcode elsewhere. This probably means explaining the structure of the passInstructions array at the beginning of the function.

[mgieseki]

Like this?

if (!append_instruction_char(passInstructions, &passIC, pass_lookback)) return 0;

Yes, exactly.

I prefer that over a macro.

Ok, sure. I can adapt the code accordingly. In case you prefer the repetitive checks over a single check at the beginning of the loop body, I'll change the PR that way. If you happen to have already adapted the code yourself, you can of course commit your changes as well.

[bertfrees]

This probably means explaining the structure of the passInstructions array at the beginning of the function.

I've done a first attempt with this table:

_	_	1
_99	_	99
!	!
`	`
~	~
/	/
"foo"	"	3	f	o	o
@12-34-45	@	3	*	*	*
[	[
]	]
#1=99	=	1	99
#1<99	<	1	99
#1<=99	130	1	99
#1>99	>	1	99
#1<=99	131	1	99
#1+	+	1
#1-	-	1
$d	$	*	*	*	*	1	1
$d.	$	*	*	*	*	1	0xffff
$d9	$	*	*	*	*	9	9
$d9-99	$	*	*	*	*	9	99
{foo	{	*	*
}foo	}	*	*
;foo	;	*	*
%foo (character class)	%	*	*	*	*	1	1
%foo.	%	*	*	*	*	1	0xffff
%foo9	%	*	*	*	*	9	9
%foo9-99	%	*	*	*	*	9	99
%foo (swap class)	%	*	*	1	1
%foo.	%	*	*	1	0xffff
%foo9	%	*	*	9	9
%foo9-99	%	*	*	9	99
*	*
?	?
(end of test, begin of action)	32 (space)

(created a new issue for this: #1215)

[mgieseki]

I've replaced the macro with a function.

[bertfrees]

Thanks.

[mgieseki]

Should I run clang-format -i on compileTranslationTable.c in order to fix the format check, or do you take care of this?

[bertfrees]

Yes you may do it, otherwise we'll take care of it.

[egli]

This comment is really confusing now. It should stay with the line above as in the original.

[bertfrees]

I've removed the comment, and also removed the passHoldNumber = passInstructions[passIC - 1]; line because it was not needed.