Re: [fricas-devel] [PATCH] fix title related memory issues
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 -- You received this message because you are subscribed to the Google Groups "FriCAS - computer algebra system" group. To unsubscribe from this group and stop receiving emails from it, send an email to fricas-devel+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/fricas-devel/ZlPUxJnSmBKcI0-r%40fricas.org.
Re: [fricas-devel] [PATCH] fix title related memory issues
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
On 5/26/24 20:05, Waldek Hebisch wrote:
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).
--
You received this message because you are subscribed to the Google Groups "FriCAS -
computer algebra system" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to fricas-devel+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/fricas-devel/f2e8714f-8d3a-44ac-aa7a-51ee4750209a%40gmail.com.
diff --git a/src/graph/include/spadAction2d.H1 b/src/graph/include/spadAction2d.H1
index 455fef4a..fd4457ad 100644
--- a/src/graph/include/spadAction2d.H1
+++ b/src/graph/include/spadAction2d.H1
@@ -1,2 +1 @@
-extern int readViewman(void *, int);
extern int spadAction(void);
diff --git a/src/graph/include/spadAction3d.H1 b/src/graph/include/spadAction3d.H1
index cc2cc432..753346ae 100644
--- a/src/graph/include/spadAction3d.H1
+++ b/src/graph/include/spadAction3d.H1
@@ -1,3 +1,2 @@
-extern int readViewman(void *, int);
extern void scalePoint(viewTriple *);
extern int spadAction(void);
diff --git a/src/graph/view2D/spadAction2d.c b/src/graph/view2D/spadAction2d.c
index a923ad88..c5d71714 100644
--- a/src/graph/view2D/spadAction2d.c
+++ b/src/graph/view2D/spadAction2d.c
@@ -44,22 +44,6 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#include "util.H1"
#include "strutil.h"
-/**
- * int readViewman(info,size) *
- **/
-
-int
-readViewman(void * info,int size)
-{
- int mold = 0;
-
- fricas_sprintf_to_buf3(errorStr, "%s %d %s", "read of ", size,
- " bytes from viewport manager\n");
- mold = check(read(0,info,size));
- return(mold);
-
-}
-
/
* int spadAction() *
/
@@ -95,8 +79,7 @@ spadAction(void)
case changeTitle:
readViewman(,intSize);
-readViewman(viewport->title,i1);
-viewport->title[i1] = '\0';
+readViewmanStr(viewport->title, i1, sizeof(viewport->title));
writeTitle();
writeControlTitle();
XFlush(dsply);
@@ -105,9 +88,7 @@ spadAction(void)
case writeView:
readViewman(,intSize);
-readViewman(filename,i1);
-filename[i1] = '\0';
-fricas_sprintf_to_buf1(errorStr, "%s", "writing of viewport data");
+readViewmanStr(filename, i1, sizeof(filename));
i3 = 0;
readViewman(,intSize);
while (i2) {
diff --git a/src/graph/view2D/viewport2D.c b/src/graph/view2D/viewport2D.c
index 95b15f21..bc428ce5 100644
--- a/src/graph/view2D/viewport2D.c
+++ b/src/graph/view2D/viewport2D.c
@@ -514,7 +514,7 @@ makeViewport(char *title,int vX,int vY,int vW,int vH,int showCP)
fprintf(stderr,"view2D: Made a viewport\n");
#endif
- strcpy(viewport->title,title);
+ strncpy(viewport->title, title, sizeof(viewport->title) - 1);
viewport->closing = no;
viewport->allowDraw= yes; /* just draw axes the first time around */
diff --git a/src/graph/view3D/header.h b/src/graph/view3D/header.h
index 21009335..622b744e 100644
--- a/src/graph/view3D/header.h
+++ b/src/graph/view3D/header.h
@@ -269,7 +269,7 @@ typedef struct _buttonStruct {
typedef struct _controlPanelStruct {
WindowcontrolWindow, messageWindow, colormapWindow;
- char message[40];
+ char message[80];
buttonStruct buttonQueue[maxButtons3D];
} controlPanelStruct;
diff --git a/src/graph/view3D/spadAction3d.c b/src/graph/view3D/spadAction3d.c
index 1cf79001..4f1c1313 100644
--- a/src/graph/view3D/spadAction3d.c
+++ b/src/graph/view3D/spadAction3d.c
@@ -46,17 +46,6 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#include "all_3d.H1"
#include "strutil.h"
-int
-readViewman (void *info,int size)
-{
- int m = 0;
-
- fricas_sprintf_to_buf1(errorStr, "%s", "read from viewport manager\n");
- m = check(read( 0, info, size));
-
- return(m);
-
-}
void
scalePoint (viewTriple *p)
Re: [fricas-devel] [PATCH] fix title related memory issues
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 -- You received this message because you are subscribed to the Google Groups "FriCAS - computer algebra system" group. To unsubscribe from this group and stop receiving emails from it, send an email to fricas-devel+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/fricas-devel/ZlMlljNQXN8q39UM%40fricas.org.
Re: [fricas-devel] [PATCH] fix title related memory issues
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 On 5/24/24 23:40, Waldek Hebisch wrote: 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? -- You received this message because you are subscribed to the Google Groups "FriCAS - computer algebra system" group. To unsubscribe from this group and stop receiving emails from it, send an email to fricas-devel+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/fricas-devel/afa5c34c-5878-4b9b-aaea-b6150ae26681%40gmail.com.
Re: [fricas-devel] [PATCH] fix title related memory issues
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 -- You received this message because you are subscribed to the Google Groups "FriCAS - computer algebra system" group. To unsubscribe from this group and stop receiving emails from it, send an email to fricas-devel+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/fricas-devel/ZlC02EwL7F1F86km%40fricas.org.
Re: [fricas-devel] [PATCH] fix title related memory issues
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 On 5/24/24 07:19, Waldek Hebisch wrote: 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. -- You received this message because you are subscribed to the Google Groups "FriCAS - computer algebra system" group. To unsubscribe from this group and stop receiving emails from it, send an email to fricas-devel+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/fricas-devel/2ee6714f-170e-49ca-810a-2c43ef1bcb94%40gmail.com.
Re: [fricas-devel] [PATCH] fix title related memory issues
On Wed, May 22, 2024 at 10:10:16PM +0800, Qian Yun wrote: > 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) 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 -- You received this message because you are subscribed to the Google Groups "FriCAS - computer algebra system" group. To unsubscribe from this group and stop receiving emails from it, send an email to fricas-devel+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/fricas-devel/Zk_O-SOn0WpvMnqQ%40fricas.org.
