Re: [Groff] problem with preconv and sample_docs.mom

[Bertrand Garrigues]

Hi Ralph,

On Sun, Nov 05 2017 at 10:05:07 AM, Ralph Corderoy <address@hidden> wrote:
>> +    /* uchardet 0.0.1 could return an empty string instead of NULL */
>> +    if (!charset[0]) {
>> +      uchardet_delete(ud);
>> +      return NULL;
>> +    }
>
> That doesn't free(data), as the code continues to do after `if (charset)
> {...}'.  If the test for charset being non-NULL isn't sufficient
> then I'd expect just that to need changing.
>
>     if (charset && *charset) {
>
> But reading
> http://git.savannah.gnu.org/cgit/groff.git/tree/src/preproc/preconv/preconv.cpp
> suggests other problems, ignoring the lack of error checking.
[...]

You are right; here is a patch to fix all that, does it look OK to you?

Regards,

Bertrand Garrigues

diff --git a/src/preproc/preconv/preconv.cpp b/src/preproc/preconv/preconv.cpp
index 97d4feb1..d84e3057 100644
--- a/src/preproc/preconv/preconv.cpp
+++ b/src/preproc/preconv/preconv.cpp
@@ -1005,56 +1005,64 @@ char *
 detect_file_encoding(FILE *fp)
 {
 #ifdef HAVE_UCHARDET
-  uchardet_t ud;
+  uchardet_t ud = NULL;
   struct stat stat_buf;
   size_t len, read_bytes;
-  char *data;
+  char *data = NULL;
   int res, current_position;
   const char *charset;
-  char *ret;
+  char *ret = NULL;
 
   current_position = ftell(fp);
   /* due to BOM and tag detection we are not at the begining of the file */
   rewind(fp);
   if (fstat(fileno(fp), &stat_buf) != 0) {
     fprintf(stderr, "fstat: %s\n", strerror(errno));
-    return NULL;
+    goto end;
   }
   len = stat_buf.st_size;
   if (debug_flag)
     fprintf(stderr, "  len: %zu\n", len);  
   if (len == 0)
-    return NULL;
+    goto end;
   data = (char *)calloc(len, 1);
   read_bytes = fread(data, 1, len, fp);
   if (read_bytes == 0) {
     fprintf(stderr, "fread: %s\n", strerror(errno));
-    return NULL;
+    goto end;
   }
   /* We rewind back to the original position */
   if (fseek(fp, current_position, SEEK_SET) != 0) {
     fprintf(stderr, "Fatal error: fseek: %s\n", strerror(errno));
-    return NULL;
+    goto end;
   }
   ud = uchardet_new();
   res = uchardet_handle_data(ud, data, len);
   if (res != 0) {
     fprintf(stderr, "uchardet_handle_data: %d\n", res);
-    uchardet_delete(ud);
-    return NULL;
+    goto end;
   }
   if (debug_flag)
     fprintf(stderr, "  uchardet read: %zu bytes\n", read_bytes);
   uchardet_data_end(ud);
   charset = uchardet_get_charset(ud);
-  if (debug_flag)
-    fprintf(stderr, "  charset: %s\n", charset);
-  if (charset) {
+  if (debug_flag) {
+    if (charset)
+       fprintf(stderr, "  charset: %s\n", charset);
+    else
+       fprintf(stderr, "  charset is NULL\n");
+  }
+  /* uchardet 0.0.1 could return an empty string instead of NULL */
+  if (charset && *charset) {
     ret = (char *)calloc(strlen(charset) + 1, 1);
     strcpy(ret, charset);
   }
-  uchardet_delete(ud);
-  free(data);
+
+end:
+  if (ud)
+     uchardet_delete(ud);
+  if (data)
+     free(data);
 
   return ret;
 #else /* not HAVE_UCHARDET */