[PATCH] fix title related memory issues

[Qian Yun]

This patch fixes many memory issues related with title --
especially with long titles in graphs.

The fix includes:

1. memory overwrite in 'spadAction'

(1) -> vp := draw(x,x=1..2); title(vp, new(100, "a".1)$String)
Compiling function %C with type DoubleFloat -> DoubleFloat
Graph data being transmitted to the viewport manager...
FriCAS2D data being transmitted to the viewport manager...
X Error of failed request: BadWindow (invalid Window parameter)
Major opcode of failed request: 3 (X_GetWindowAttributes)
Resource id in failed request: 0x61616161
Serial number of failed request: 777
Current serial number in output stream: 778

2. buffer overflow in 'makeViewport'

3. memory leakage in 'funView2D'/'funView3D'

(if you run "title" in a loop)

- Qian

[Waldek Hebisch]

It is good to fix buffer overflow. But you also remove error
checking (use of 'check'). Admited, this is minimal error
checking, just printing error message, but this is better than
nothing.

--
Waldek Hebisch

[Qian Yun]

I wanted to avoid pulling in "fricas_sprintf_to_buf".

Turns out I can use

const char *errorStr = "read from viewport manager\n";

Are you OK with this?

- Qian

[Waldek Hebisch]

On Fri, May 24, 2024 at 05:04:08PM +0800, Qian Yun wrote:
> I wanted to avoid pulling in "fricas_sprintf_to_buf".
>
> Turns out I can use
>
> const char *errorStr = "read from viewport manager\n";
>
> Are you OK with this?

OK if we really need to remove uses of "fricas_sprintf_to_buf".
But why do you want to remove such uses?

--
Waldek Hebisch

[Qian Yun]

Sorry, the problem is not "fricas_sprintf_to_buf", but
"extern char errorStr [80];".

I want to avoid this global variable and implementation
detail to leak to util.c.

- Qian

[Waldek Hebisch]

On Sat, May 25, 2024 at 06:27:10PM +0800, Qian Yun wrote:
> Sorry, the problem is not "fricas_sprintf_to_buf", but
> "extern char errorStr [80];".
>
> I want to avoid this global variable and implementation
> detail to leak to util.c.

So you are trying to clean up the code. I must say that
I would prefer to have 'checked' read that would loop in
case of short read (usually 'read' delivers all requested
data, but it can return only part of what was requested
in in such case we should loop (this is not handled in
current code)). Such 'checked' read would get parameters
needed to build error message (so no global buffer and no
'sprintf' at call site).


--
Waldek Hebisch

[Qian Yun]

See my updated attachment.

In this version I did not try to do cleanups, instead I
declare errorStr as local variable.

As for "check", I find that it gets redefined in
graph/viewAlone/viewAlone.h, graph/viewman/viewman.h:

#define check(code) checker(code,__LINE__,"")

Also I don't like its overhead when we only read a few bytes,
I hope in the future we can avoid the overhead if we are
building in release mode (aka when there is no DEBUG defined).

- Qian

[Waldek Hebisch]

On Sun, May 26, 2024 at 09:04:27PM +0800, Qian Yun wrote:
> See my updated attachment.
>
> In this version I did not try to do cleanups, instead I
> declare errorStr as local variable.
>
> As for "check", I find that it gets redefined in
> graph/viewAlone/viewAlone.h, graph/viewman/viewman.h:
>
> #define check(code) checker(code,__LINE__,"")
>
> Also I don't like its overhead when we only read a few bytes,
> I hope in the future we can avoid the overhead if we are
> building in release mode (aka when there is no DEBUG defined).

OK.

--
Waldek Hebisch