PATCH: Lua 5.0.2 popen/pclose fix (was Re: segfault in io.close and a plea for pipes)

[Mike Pall <mikelu-0505@...>]

Hi,

David Given wrote:
> On Monday 23 May 2005 03:33, William Trenker wrote:
> [...]
> >     int ok = (pclose(f) != -1) || (fclose(f) == 0);     // <<<---
> > PROBLEM IS HERE
> 
> Yes, this is a known problem (it fails on my platform, too). It's actually a 
> Posix violation, but just happens to work on most platforms.
> 
> The problem is that fixing would require some pretty hefty rewriting of the 
> way files are managed (because you need to store whether the FILE* came from 
> popen() or fopen(), which means you need another word of storage, which means 
> you can't store the FILE* in a lightuserdata), and, well, no-one's got around 
> to doing it yet.

Nope, it's already a userdata object. Patch against Lua 5.0.2 attached.

I backported a few error handling fixes from Lua 5.1, too. I also
fixed the semi-random error message you get from io.stdout:close() --
closing io.stdin/stdout/stderr is ignored now.

BTW: Yes, the latter bug is still present in Lua 5.1-work6:
     $ lua -e 'print(io.stdout:close())'
     nil     Success 0
     $ lua -e 'io.open("doesnotexist"); print(io.stdout:close())'
     nil     No such file or directory       2

Bye,
     Mike

--- lua-5.0.2/src/lib/liolib.c	2003-10-01 15:16:49.000000000 +0200
+++ lua-5.0.2-popen/src/lib/liolib.c	2005-05-23 21:18:08.000000000 +0200
@@ -68,6 +68,12 @@
 #define IO_INPUT		"_input"
 #define IO_OUTPUT		"_output"
 
+typedef struct FHandle {
+  FILE *f;
+  int noclose;
+  int ispipe;
+} FHandle;
+
 
 static int pushresult (lua_State *L, int i, const char *filename) {
   if (i) {
@@ -86,29 +92,28 @@
 }
 
 
-static FILE **topfile (lua_State *L, int findex) {
-  FILE **f = (FILE **)luaL_checkudata(L, findex, FILEHANDLE);
-  if (f == NULL) luaL_argerror(L, findex, "bad file");
-  return f;
+static FHandle *tofhandle (lua_State *L, int findex) {
+  FHandle *fh = (FHandle *)luaL_checkudata(L, findex, FILEHANDLE);
+  if (fh == NULL) luaL_argerror(L, findex, "bad file");
+  return fh;
 }
 
 
 static int io_type (lua_State *L) {
-  FILE **f = (FILE **)luaL_checkudata(L, 1, FILEHANDLE);
-  if (f == NULL) lua_pushnil(L);
-  else if (*f == NULL)
-    lua_pushliteral(L, "closed file");
+  FHandle *fh = (FHandle *)luaL_checkudata(L, 1, FILEHANDLE);
+  if (fh == NULL) lua_pushnil(L);
   else
-    lua_pushliteral(L, "file");
+    lua_pushstring(L, fh->ispipe ? (fh->f ? "pipe" : "closed pipe") :
+                                   (fh->f ? "file" : "closed file"));
   return 1;
 }
 
 
 static FILE *tofile (lua_State *L, int findex) {
-  FILE **f = topfile(L, findex);
-  if (*f == NULL)
+  FHandle *fh = tofhandle(L, findex);
+  if (fh->f == NULL)
     luaL_error(L, "attempt to use a closed file");
-  return *f;
+  return fh->f;
 }
 
 
@@ -118,12 +123,14 @@
 ** before opening the actual file; so, if there is a memory error, the
 ** file is not left opened.
 */
-static FILE **newfile (lua_State *L) {
-  FILE **pf = (FILE **)lua_newuserdata(L, sizeof(FILE *));
-  *pf = NULL;  /* file handle is currently `closed' */
+static FHandle *newfile (lua_State *L) {
+  FHandle *fh = (FHandle *)lua_newuserdata(L, sizeof(FHandle));
+  fh->f = NULL;  /* file handle is currently `closed' */
+  fh->ispipe = 0;
+  fh->noclose = 0;
   luaL_getmetatable(L, FILEHANDLE);
   lua_setmetatable(L, -2);
-  return pf;
+  return fh;
 }
 
 
@@ -133,8 +140,11 @@
 */
 static void registerfile (lua_State *L, FILE *f, const char *name,
                                                  const char *impname) {
+  FHandle *fh;
   lua_pushstring(L, name);
-  *newfile(L) = f;
+  fh = newfile(L);
+  fh->f = f;
+  fh->noclose = 1;
   if (impname) {
     lua_pushstring(L, impname);
     lua_pushvalue(L, -2);
@@ -144,16 +154,14 @@
 }
 
 
-static int aux_close (lua_State *L) {
-  FILE *f = tofile(L, 1);
-  if (f == stdin || f == stdout || f == stderr)
-    return 0;  /* file cannot be closed */
-  else {
-    int ok = (pclose(f) != -1) || (fclose(f) == 0);
-    if (ok)
-      *(FILE **)lua_touserdata(L, 1) = NULL;  /* mark file as closed */
-    return ok;
-  }
+static int aux_close (lua_State *L, FHandle *fh) {
+  FILE *f = fh->f;
+  if (fh->noclose)
+    return 1;  /* file cannot be closed (but return ok) */
+  if (f == NULL)
+    luaL_error(L, "attempt to close a file twice");
+  fh->f = NULL;  /* mark file as closed */
+  return fh->ispipe ? (pclose(f) != -1) : (fclose(f) == 0);
 }
 
 
@@ -162,25 +170,25 @@
     lua_pushstring(L, IO_OUTPUT);
     lua_rawget(L, lua_upvalueindex(1));
   }
-  return pushresult(L, aux_close(L), NULL);
+  return pushresult(L, aux_close(L, tofhandle(L, 1)), NULL);
 }
 
 
 static int io_gc (lua_State *L) {
-  FILE **f = topfile(L, 1);
-  if (*f != NULL)  /* ignore closed files */
-    aux_close(L);
+  FHandle *fh = tofhandle(L, 1);
+  if (fh->f != NULL)  /* ignore closed files */
+    aux_close(L, fh);
   return 0;
 }
 
 
 static int io_tostring (lua_State *L) {
   char buff[128];
-  FILE **f = topfile(L, 1);
-  if (*f == NULL)
+  FHandle *fh = tofhandle(L, 1);
+  if (fh->f == NULL)
     strcpy(buff, "closed");
   else
-    sprintf(buff, "%p", lua_touserdata(L, 1));
+    sprintf(buff, "%p", fh->f);
   lua_pushfstring(L, "file (%s)", buff);
   return 1;
 }
@@ -189,9 +197,9 @@
 static int io_open (lua_State *L) {
   const char *filename = luaL_checkstring(L, 1);
   const char *mode = luaL_optstring(L, 2, "r");
-  FILE **pf = newfile(L);
-  *pf = fopen(filename, mode);
-  return (*pf == NULL) ? pushresult(L, 0, filename) : 1;
+  FHandle *fh = newfile(L);
+  fh->f = fopen(filename, mode);
+  return (fh->f == NULL) ? pushresult(L, 0, filename) : 1;
 }
 
 
@@ -202,17 +210,18 @@
 #else
   const char *filename = luaL_checkstring(L, 1);
   const char *mode = luaL_optstring(L, 2, "r");
-  FILE **pf = newfile(L);
-  *pf = popen(filename, mode);
-  return (*pf == NULL) ? pushresult(L, 0, filename) : 1;
+  FHandle *fh = newfile(L);
+  fh->f = popen(filename, mode);
+  fh->ispipe = 1;
+  return (fh->f == NULL) ? pushresult(L, 0, filename) : 1;
 #endif
 }
 
 
 static int io_tmpfile (lua_State *L) {
-  FILE **pf = newfile(L);
-  *pf = tmpfile();
-  return (*pf == NULL) ? pushresult(L, 0, NULL) : 1;
+  FHandle *fh = newfile(L);
+  fh->f = tmpfile();
+  return (fh->f == NULL) ? pushresult(L, 0, NULL) : 1;
 }
 
 
@@ -228,9 +237,9 @@
     const char *filename = lua_tostring(L, 1);
     lua_pushstring(L, name);
     if (filename) {
-      FILE **pf = newfile(L);
-      *pf = fopen(filename, mode);
-      if (*pf == NULL) {
+      FHandle *fh = newfile(L);
+      fh->f = fopen(filename, mode);
+      if (fh->f == NULL) {
         lua_pushfstring(L, "%s: %s", filename, strerror(errno));
         luaL_argerror(L, 1, lua_tostring(L, -1));
       }
@@ -261,11 +270,11 @@
 static int io_readline (lua_State *L);
 
 
-static void aux_lines (lua_State *L, int idx, int close) {
+static void aux_lines (lua_State *L, int idx, int toclose) {
   lua_pushliteral(L, FILEHANDLE);
   lua_rawget(L, LUA_REGISTRYINDEX);
   lua_pushvalue(L, idx);
-  lua_pushboolean(L, close);  /* close/not close file when finished */
+  lua_pushboolean(L, toclose);  /* close/not close file when finished */
   lua_pushcclosure(L, io_readline, 3);
 }
 
@@ -285,9 +294,9 @@
   }
   else {
     const char *filename = luaL_checkstring(L, 1);
-    FILE **pf = newfile(L);
-    *pf = fopen(filename, "r");
-    luaL_argcheck(L, *pf, 1,  strerror(errno));
+    FHandle *fh = newfile(L);
+    fh->f = fopen(filename, "r");
+    luaL_argcheck(L, fh->f, 1,  strerror(errno));
     aux_lines(L, lua_gettop(L), 1);
     return 1;
   }
@@ -363,6 +372,7 @@
   int nargs = lua_gettop(L) - 1;
   int success;
   int n;
+  clearerr(f);
   if (nargs == 0) {  /* no arguments? */
     success = read_line(L, f);
     n = first+1;  /* to return 1 result */
@@ -397,6 +407,8 @@
       }
     }
   }
+  if (ferror(f))
+    return pushresult(L, 0, NULL);
   if (!success) {
     lua_pop(L, 1);  /* remove last result */
     lua_pushnil(L);  /* push nil instead */
@@ -416,16 +428,17 @@
 
 
 static int io_readline (lua_State *L) {
-  FILE *f = *(FILE **)lua_touserdata(L, lua_upvalueindex(2));
-  if (f == NULL)  /* file is already closed? */
+  FHandle *fh = (FHandle *)lua_touserdata(L, lua_upvalueindex(2));
+  int success;
+  if (fh->f == NULL)  /* file is already closed? */
     luaL_error(L, "file is already closed");
-  if (read_line(L, f)) return 1;
+  success = read_line(L, fh->f);
+  if (ferror(fh->f))
+    luaL_error(L, "%s", strerror(errno));
+  if (success) return 1;
   else {  /* EOF */
-    if (lua_toboolean(L, lua_upvalueindex(3))) {  /* generator created file? */
-      lua_settop(L, 0);
-      lua_pushvalue(L, lua_upvalueindex(2));
-      aux_close(L);  /* close it */
-    }
+    if (lua_toboolean(L, lua_upvalueindex(3)))  /* generator created file? */
+      aux_close(L, fh);  /* close it */
     return 0;
   }
 }