[Gcl-devel] SGC and file I/O

[Camm Maguire]

Greetings!  About a year ago, I quickly plugged an error in 2.5.0
which showed up in compiling acl2 on m68k.  Briefly, if one uses
read() or fread() to write directly to memory that has been mprotected
read-only when sgc is enabled, the memprotect_handler signal handler
will never be invoked, the memory area will never be re-protected
read-write, and the read()/fread() system calls will simply fail
returning an error condition.  My solution at that time was to take
the offending fread (only) and preface it with a memset to initialize
the buffer, and trigger any possible segfaults, in advance of fread.

I wasn't very systematic at the time, but just wanted to get the
current stable working.  I have a similar error in building acl2 on
m68k with the latest stable release candidate which I have thus far
been unable to reproduce.  Nevertheless, my best guess at this moment
is that another fread has bitten the dust.  More diagnostics will
follow. 

I think it prudent in any case to protect read and fread wherever this
problem could occur.  I have a patch which I am testing and will
commit if all goes well.  Here's the idea:

Index: h/notcomp.h
===================================================================
RCS file: /cvsroot/gcl/gcl/h/notcomp.h,v
retrieving revision 1.15.6.2
diff -u -b -r1.15.6.2 notcomp.h
--- h/notcomp.h 6 Nov 2003 16:16:49 -0000       1.15.6.2
+++ h/notcomp.h 11 Feb 2004 21:44:08 -0000
@@ -270,3 +270,18 @@
 
 int
 gcl_init_cmp_anon(void);
+
+#undef SAFE_READ
+#undef SAFE_FREAD
+#ifdef SGC
+#define SAFE_READ(a_,b_,c_) \
+   ({int _a=(a_),_c=(c_);char *_b=(b_);extern int sgc_enabled;\
+     if (sgc_enabled) memset(_b,0,_c);read(_a,_b,_c);})
+#define SAFE_FREAD(a_,b_,c_,d_) \
+   ({int _b=(b_),_c=(c_);char *_a=(a_);FILE *_d=(d_);extern int sgc_enabled; \
+     if (sgc_enabled) memset(_a,0,_b*_c);fread(_a,_b,_c,_d);})
+#else
+#define SAFE_READ(a_,b_,c_) read((a_),(b_),(c_))
+#define SAFE_FREAD(a_,b_,c_,d_) fread((a_),(b_),(c_),(d_))
+#endif
+

And then

Index: o/file.d
===================================================================
RCS file: /cvsroot/gcl/gcl/o/file.d,v
retrieving revision 1.21.4.1.2.4
diff -u -b -r1.21.4.1.2.4 file.d
--- o/file.d    6 Nov 2003 16:16:50 -0000       1.21.4.1.2.4
+++ o/file.d    11 Feb 2004 21:44:09 -0000
@@ -2130,7 +2130,7 @@
   p = vector->ust.ust_self;
   beg = ((type_of(start)==t_fixnum) ? fix(start) : 0);
   n = ((type_of(count)==t_fixnum) ? fix(count) : (vector->st.st_fillp - beg));
-  if ((n=fread(p+beg,1,n,stream->sm.sm_fp)))
+  if ((n=SAFE_FREAD(p+beg,1,n,stream->sm.sm_fp)))
       @(return `make_fixnum(n)`);
   @(return Cnil);
 @)


etc.  This only needs to be done when read or fread is writing to
memory allocated by the memory management system -- i.e. uses with
local buffers allocated on the stack need not make this modification.
Strictly speaking, memory returned by malloc does not require this
either, as these pages are marked permanently writable if sgc is on,
but as we may change this in the future, I think malloced memory
should be protected too.  

In my build, 'nm' shows the following dependencies on read/fread:

$ for i in *.o; do if nm $i | grep -i '\<fread\>' ; then echo $i ; fi ; done
         U fread
fat_string.o
         U fread
file.o
         U fread
sfasl.o
$ for i in *.o; do if nm $i | grep -i '\<read\>' ; then echo $i ; fi ; done
         U read
nsocket.o
         U read
sockets.o

I've gone through these files and identified the locations where I
think this patch is needed, at least on linux.  It would be good if
the maintainers of the other ports (esp. windows, macosx, xbsd) could
double check and make sure nothing has been left out in their platform
specific code. 

Here is the whole patch, again as yet uncommitted:

Index: gcl-tk/comm.c
===================================================================
RCS file: /cvsroot/gcl/gcl/gcl-tk/comm.c,v
retrieving revision 1.5.6.2
diff -u -b -r1.5.6.2 comm.c
--- gcl-tk/comm.c       6 Nov 2003 16:16:49 -0000       1.5.6.2
+++ gcl-tk/comm.c       11 Feb 2004 21:44:08 -0000
@@ -186,7 +186,7 @@
   if ((fix(FFN(fScheck_fd_for_input)(sfd->fd,sfd->write_timeout))>0));
      again:
         {char *start = sfd->valid_data+sfd->valid_data_size;
-        nread = read(sfd->fd,start,
+        nread = SAFE_READ(sfd->fd,start,
                     sfd->read_buffer_size - (start -  sfd->read_buffer));
        }
         if (nread<0)
Index: gcl-tk/guis.c
===================================================================
RCS file: /cvsroot/gcl/gcl/gcl-tk/guis.c,v
retrieving revision 1.6.6.1
diff -u -b -r1.6.6.1 guis.c
--- gcl-tk/guis.c       26 Sep 2003 13:46:54 -0000      1.6.6.1
+++ gcl-tk/guis.c       11 Feb 2004 21:44:08 -0000
@@ -197,7 +197,9 @@
    (giving a human debugger time to attach to the forked process).
  */
 
-
+#ifdef SGC
+int sgc_enabled=0;
+#endif
 
 int delay;
 int main(argc, argv,envp)
Index: h/notcomp.h
===================================================================
RCS file: /cvsroot/gcl/gcl/h/notcomp.h,v
retrieving revision 1.15.6.2
diff -u -b -r1.15.6.2 notcomp.h
--- h/notcomp.h 6 Nov 2003 16:16:49 -0000       1.15.6.2
+++ h/notcomp.h 11 Feb 2004 21:44:08 -0000
@@ -270,3 +270,18 @@
 
 int
 gcl_init_cmp_anon(void);
+
+#undef SAFE_READ
+#undef SAFE_FREAD
+#ifdef SGC
+#define SAFE_READ(a_,b_,c_) \
+   ({int _a=(a_),_c=(c_);char *_b=(b_);extern int sgc_enabled;\
+     if (sgc_enabled) memset(_b,0,_c);read(_a,_b,_c);})
+#define SAFE_FREAD(a_,b_,c_,d_) \
+   ({int _b=(b_),_c=(c_);char *_a=(a_);FILE *_d=(d_);extern int sgc_enabled; \
+     if (sgc_enabled) memset(_a,0,_b*_c);fread(_a,_b,_c,_d);})
+#else
+#define SAFE_READ(a_,b_,c_) read((a_),(b_),(c_))
+#define SAFE_FREAD(a_,b_,c_,d_) fread((a_),(b_),(c_),(d_))
+#endif
+
Index: o/fasdump.c
===================================================================
RCS file: /cvsroot/gcl/gcl/o/fasdump.c,v
retrieving revision 1.9.6.1
diff -u -b -r1.9.6.1 fasdump.c
--- o/fasdump.c 6 Nov 2003 16:16:50 -0000       1.9.6.1
+++ o/fasdump.c 11 Feb 2004 21:44:08 -0000
@@ -262,7 +262,7 @@
      int n1;
      int n2;
 {int i,j;
- j=fread(p,n1,n2,st);
+ j=SAFE_FREAD(p,n1,n2,st);
  if(debug)
  {printf("[");
   n1=n1*n2;
@@ -317,7 +317,7 @@
 #define GET_OP GET
 #define PUT_OP PUT
 #define D_FWRITE fwrite
-#define D_FREAD fread
+#define D_FREAD SAFE_FREAD
 #define DP(sw)  sw
 #define PUTD(a,b) PUT(b)
 #define GETD(a) GET()
@@ -449,7 +449,8 @@
 #define  READ_STRING(leng,loc) do {BEGIN_NO_INTERRUPT;     \
      *loc = alloc_simple_string(leng); \
      (*loc)->st.st_self=alloc_relblock(leng); END_NO_INTERRUPT; \
-  memset((*loc)->st.st_self,0,leng); /* fread won't restart if it triggers an 
SGC segfault -- CM */ \
+/* Now handled in SAFE_FREAD -- CM 20040210 */ \
+/*   memset((*loc)->st.st_self,0,leng); */ /* fread won't restart if it 
triggers an SGC segfault -- CM */ \
   D_FREAD((*loc)->st.st_self,1,leng,fas_stream);} while(0)
 
 /* if try_hash finds it we don't need to write the object
Index: o/file.d
===================================================================
RCS file: /cvsroot/gcl/gcl/o/file.d,v
retrieving revision 1.21.4.1.2.4
diff -u -b -r1.21.4.1.2.4 file.d
--- o/file.d    6 Nov 2003 16:16:50 -0000       1.21.4.1.2.4
+++ o/file.d    11 Feb 2004 21:44:09 -0000
@@ -2130,7 +2130,7 @@
   p = vector->ust.ust_self;
   beg = ((type_of(start)==t_fixnum) ? fix(start) : 0);
   n = ((type_of(count)==t_fixnum) ? fix(count) : (vector->st.st_fillp - beg));
-  if ((n=fread(p+beg,1,n,stream->sm.sm_fp)))
+  if ((n=SAFE_FREAD(p+beg,1,n,stream->sm.sm_fp)))
       @(return `make_fixnum(n)`);
   @(return Cnil);
 @)
Index: o/nsocket.c
===================================================================
RCS file: /cvsroot/gcl/gcl/o/nsocket.c,v
retrieving revision 1.7.4.1.4.1
diff -u -b -r1.7.4.1.4.1 nsocket.c
--- o/nsocket.c 6 Nov 2003 16:16:52 -0000       1.7.4.1.4.1
+++ o/nsocket.c 11 Feb 2004 21:44:09 -0000
@@ -635,7 +635,7 @@
       if (high > 0)
        { object bufp = SOCKET_STREAM_BUFFER(strm);
        int n;
-       n = read(fd,bufp->ust.ust_self ,bufp->ust.ust_dim);
+       n = SAFE_READ(fd,bufp->ust.ust_self ,bufp->ust.ust_dim);
        doReverse(bufp->ust.ust_self,n);
        bufp->ust.ust_fillp=n;
        if (n > 0)
Index: o/sfasl.c
===================================================================
RCS file: /cvsroot/gcl/gcl/o/sfasl.c,v
retrieving revision 1.13
diff -u -b -r1.13 sfasl.c
--- o/sfasl.c   18 Feb 2003 02:32:03 -0000      1.13
+++ o/sfasl.c   11 Feb 2004 21:44:09 -0000
@@ -254,7 +254,7 @@
        dprintf( code size %d , datasize+textsize+bsssize + extra_bss);
        if (fseek(fp,N_TXTOFF(fileheader) ,0) < 0)
                FEerror("file seek error",0,0);
-       fread(the_start, textsize + datasize, 1, fp);
+       SAFE_FREAD(the_start, textsize + datasize, 1, fp);
        dprintf(read into memory text +data %d bytes, textsize + datasize);
 /* relocate the actual loaded text  */
 
Index: o/sfaslelf.c
===================================================================
RCS file: /cvsroot/gcl/gcl/o/sfaslelf.c,v
retrieving revision 1.8.6.1
diff -u -b -r1.8.6.1 sfaslelf.c
--- o/sfaslelf.c        16 Jan 2004 16:57:46 -0000      1.8.6.1
+++ o/sfaslelf.c        11 Feb 2004 21:44:09 -0000
@@ -204,7 +204,7 @@
      base = mmap (0, stat_buf.st_size,PROT_READ|PROT_WRITE, MAP_PRIVATE, file, 
0); } else {
 
    base = malloc(stat_buf.st_size);
-   fread(base,1,stat_buf.st_size,fp);
+   SAFE_FREAD(base,1,stat_buf.st_size,fp);
    fseek(fp,0,SEEK_CUR);
      }
 


-- 
Camm Maguire                                            address@hidden
==========================================================================
"The earth is but one country, and mankind its citizens."  --  Baha'u'llah