Get rid of gotos in CPDF_SyntaxParser and FlateUncompress().

core/src/fxcodec/codec/fx_codec_flate.cpp

Index: core/src/fxcodec/codec/fx_codec_flate.cpp
diff --git a/core/src/fxcodec/codec/fx_codec_flate.cpp b/core/src/fxcodec/codec/fx_codec_flate.cpp
index e8878e453524d58e28c991315a9189c3582d8cdb..a69b3c7118cf3e0c075a9b7194ba099cb34e2929 100644
--- a/core/src/fxcodec/codec/fx_codec_flate.cpp
+++ b/core/src/fxcodec/codec/fx_codec_flate.cpp
@@ -75,6 +75,8 @@ void FPDFAPI_FlateEnd(void* context) {
 }
 }  // extern "C"
 
+namespace {
+
 class CLZWDecoder {
  public:
   int Decode(uint8_t* output,
@@ -222,7 +224,8 @@ int CLZWDecoder::Decode(uint8_t* dest_buf,
   src_size = (m_InPos + 7) / 8;
   return 0;
 }
-static uint8_t PaethPredictor(int a, int b, int c) {
+
+uint8_t PaethPredictor(int a, int b, int c) {
   int p = a + b - c;
   int pa = FXSYS_abs(p - a);
   int pb = FXSYS_abs(p - b);
@@ -235,12 +238,13 @@ static uint8_t PaethPredictor(int a, int b, int c) {
   }
   return (uint8_t)c;
 }
-static FX_BOOL PNG_PredictorEncode(uint8_t*& data_buf,
-                                   FX_DWORD& data_size,
-                                   int predictor,
-                                   int Colors,
-                                   int BitsPerComponent,
-                                   int Columns) {
+
+FX_BOOL PNG_PredictorEncode(uint8_t*& data_buf,
+                            FX_DWORD& data_size,
+                            int predictor,
+                            int Colors,
+                            int BitsPerComponent,
+                            int Columns) {
   const int BytesPerPixel = (Colors * BitsPerComponent + 7) / 8;
   const int row_size = (Colors * BitsPerComponent * Columns + 7) / 8;
   if (row_size <= 0)
@@ -324,12 +328,13 @@ static FX_BOOL PNG_PredictorEncode(uint8_t*& data_buf,
               (last_row_size > 0 ? (row_size - last_row_size) : 0);
   return TRUE;
 }
-static void PNG_PredictLine(uint8_t* pDestData,
-                            const uint8_t* pSrcData,
-                            const uint8_t* pLastLine,
-                            int bpc,
-                            int nColors,
-                            int nPixels) {
+
+void PNG_PredictLine(uint8_t* pDestData,
+                     const uint8_t* pSrcData,
+                     const uint8_t* pLastLine,
+                     int bpc,
+                     int nColors,
+                     int nPixels) {
   int row_size = (nPixels * bpc * nColors + 7) / 8;
   int BytesPerPixel = (bpc * nColors + 7) / 8;
   uint8_t tag = pSrcData[0];
@@ -390,11 +395,12 @@ static void PNG_PredictLine(uint8_t* pDestData,
     }
   }
 }
-static FX_BOOL PNG_Predictor(uint8_t*& data_buf,
-                             FX_DWORD& data_size,
-                             int Colors,
-                             int BitsPerComponent,
-                             int Columns) {
+
+FX_BOOL PNG_Predictor(uint8_t*& data_buf,
+                      FX_DWORD& data_size,
+                      int Colors,
+                      int BitsPerComponent,
+                      int Columns) {
   const int BytesPerPixel = (Colors * BitsPerComponent + 7) / 8;
   const int row_size = (Colors * BitsPerComponent * Columns + 7) / 8;
   if (row_size <= 0)
@@ -481,11 +487,12 @@ static FX_BOOL PNG_Predictor(uint8_t*& data_buf,
               (last_row_size > 0 ? (row_size + 1 - last_row_size) : 0);
   return TRUE;
 }
-static void TIFF_PredictorEncodeLine(uint8_t* dest_buf,
-                                     int row_size,
-                                     int BitsPerComponent,
-                                     int Colors,
-                                     int Columns) {
+
+void TIFF_PredictorEncodeLine(uint8_t* dest_buf,
+                              int row_size,
+                              int BitsPerComponent,
+                              int Colors,
+                              int Columns) {
   int BytesPerPixel = BitsPerComponent * Colors / 8;
   if (BitsPerComponent < 8) {
     uint8_t mask = 0x01;
@@ -526,11 +533,12 @@ static void TIFF_PredictorEncodeLine(uint8_t* dest_buf,
     }
   }
 }
-static FX_BOOL TIFF_PredictorEncode(uint8_t*& data_buf,
-                                    FX_DWORD& data_size,
-                                    int Colors,
-                                    int BitsPerComponent,
-                                    int Columns) {
+
+FX_BOOL TIFF_PredictorEncode(uint8_t*& data_buf,
+                             FX_DWORD& data_size,
+                             int Colors,
+                             int BitsPerComponent,
+                             int Columns) {
   int row_size = (Colors * BitsPerComponent * Columns + 7) / 8;
   if (row_size == 0)
     return FALSE;
@@ -546,11 +554,12 @@ static FX_BOOL TIFF_PredictorEncode(uint8_t*& data_buf,
   }
   return TRUE;
 }
-static void TIFF_PredictLine(uint8_t* dest_buf,
-                             int row_size,
-                             int BitsPerComponent,
-                             int Colors,
-                             int Columns) {
+
+void TIFF_PredictLine(uint8_t* dest_buf,
+                      int row_size,
+                      int BitsPerComponent,
+                      int Colors,
+                      int Columns) {
   if (BitsPerComponent == 1) {
     int row_bits = FX_MIN(BitsPerComponent * Colors * Columns, row_size * 8);
     int index_pre = 0;
@@ -584,11 +593,12 @@ static void TIFF_PredictLine(uint8_t* dest_buf,
     }
   }
 }
-static FX_BOOL TIFF_Predictor(uint8_t*& data_buf,
-                              FX_DWORD& data_size,
-                              int Colors,
-                              int BitsPerComponent,
-                              int Columns) {
+
+FX_BOOL TIFF_Predictor(uint8_t*& data_buf,
+                       FX_DWORD& data_size,
+                       int Colors,
+                       int BitsPerComponent,
+                       int Columns) {
   int row_size = (Colors * BitsPerComponent * Columns + 7) / 8;
   if (row_size == 0)
     return FALSE;
@@ -604,6 +614,125 @@ static FX_BOOL TIFF_Predictor(uint8_t*& data_buf,
   return TRUE;
 }
 
+void FlateUncompress(const uint8_t* src_buf,

[Lei Zhang, 2015/09/23 19:22:28]

A bit harder to see what I changed since I moved this function.

- With the gotos gone, I can declare variables closer to where they are used.
- FX_Realloc() can't fail and return NULL, so those checks are gone.
- Call FPDFAPI_FlateEnd(context) on failure.
- Use a unique_ptr for |guess_buf| so there's slightly better tracking of
ownership.

+                     FX_DWORD src_size,
+                     FX_DWORD orig_size,
+                     uint8_t*& dest_buf,
+                     FX_DWORD& dest_size,
+                     FX_DWORD& offset) {
+  FX_DWORD guess_size = orig_size ? orig_size : src_size * 2;
+  FX_DWORD alloc_step =
+      orig_size ? 10240 : (src_size < 10240 ? 10240 : src_size);

[Tom Sepez, 2015/09/23 21:48:41]

nit: maybe std::min(src_size, 10240)
maybe a const FX_DWORD kMinStepSize = 10240;

[Lei Zhang, 2015/09/25 09:33:49]

Done.

+  static const FX_DWORD kMaxInitialAllocSize = 10000000;
+  if (guess_size > kMaxInitialAllocSize) {
+    guess_size = kMaxInitialAllocSize;
+    alloc_step = kMaxInitialAllocSize;
+  }
+  FX_DWORD buf_size = guess_size;
+  FX_DWORD last_buf_size = buf_size;
+
+  void* context = FPDFAPI_FlateInit(my_alloc_func, my_free_func);
+  if (!context) {
+    dest_buf = nullptr;

[Tom Sepez, 2015/09/23 21:48:41]

how about we set these to null/0 initially, and then only set them to legit
values right before a successful return?  Then this can be if (!context) return;

[Lei Zhang, 2015/09/25 09:33:48]

Done.

+    dest_size = 0;
+    return;
+  }
+
+  nonstd::unique_ptr<uint8_t, FxFreeDeleter> guess_buf(
+      FX_Alloc(uint8_t, guess_size + 1));
+  guess_buf.get()[guess_size] = '\0';

[Tom Sepez, 2015/09/23 21:48:41]

The real std::unique_ptr has operator[]. do we want to do the same before
landing this?  Do we want to new[] rather than mallocating?

[Lei Zhang, 2015/09/25 09:33:48]

There's no new equivalent to FX_Realloc(), right? I would use a std::vector if
we didn't have to send the |guess_buf| owned pointer out via |dest_buf|.

[Tom Sepez, 2015/09/25 16:25:34]

ah.

+
+  FPDFAPI_FlateInput(context, src_buf, src_size);
+
+  if (src_size < 10240) {
+    // This is the old implementation.
+    uint8_t* cur_buf = guess_buf.get();
+    while (1) {
+      int32_t ret = FPDFAPI_FlateOutput(context, cur_buf, buf_size);
+      if (ret != Z_OK)
+        break;
+      int32_t avail_buf_size = FPDFAPI_FlateGetAvailOut(context);
+      if (avail_buf_size != 0)
+        break;
+
+      // |avail_buf_size| == 0 case.

[Tom Sepez, 2015/09/23 21:48:41]

nit: not sure this comment adds value.

[Lei Zhang, 2015/09/25 09:33:48]

Done.

+      FX_DWORD old_size = guess_size;
+      guess_size += alloc_step;
+      if (guess_size < old_size || guess_size + 1 < guess_size) {
+        FPDFAPI_FlateEnd(context);
+        dest_buf = nullptr;
+        dest_size = 0;
+        return;
+      }
+
+      {
+        uint8_t* new_buf =
+            FX_Realloc(uint8_t, guess_buf.release(), guess_size + 1);
+        guess_buf.reset(new_buf);

[Tom Sepez, 2015/09/23 21:48:41]

nit: how about guess_buf.reset(FX_Realloc(...)); and lose the { }

[Lei Zhang, 2015/09/25 09:33:48]

No, we get in trouble doing guess_buf.reset() and guess_buf.release() at the
same time.

[Tom Sepez, 2015/09/25 16:25:34]

Acknowledged.

+      }
+      guess_buf.get()[guess_size] = '\0';
+      cur_buf = guess_buf.get() + old_size;
+      buf_size = guess_size - old_size;
+    }
+    dest_size = FPDFAPI_FlateGetTotalOut(context);
+    offset = FPDFAPI_FlateGetTotalIn(context);
+    if (guess_size / 2 > dest_size) {
+      {
+        uint8_t* new_buf =

[Tom Sepez, 2015/09/23 21:48:41]

ditto

[Lei Zhang, 2015/09/25 09:33:48]

Acknowledged.

+            FX_Realloc(uint8_t, guess_buf.release(), dest_size + 1);
+        guess_buf.reset(new_buf);
+      }
+      guess_size = dest_size;
+      guess_buf.get()[guess_size] = '\0';
+    }
+    dest_buf = guess_buf.release();
+  } else {
+    CFX_ArrayTemplate<uint8_t*> result_tmp_bufs;
+    uint8_t* cur_buf = guess_buf.release();
+    while (1) {
+      int32_t ret = FPDFAPI_FlateOutput(context, cur_buf, buf_size);
+      int32_t avail_buf_size = FPDFAPI_FlateGetAvailOut(context);
+      if (ret != Z_OK) {
+        last_buf_size = buf_size - avail_buf_size;
+        result_tmp_bufs.Add(cur_buf);
+        break;
+      }
+      if (avail_buf_size != 0) {
+        last_buf_size = buf_size - avail_buf_size;
+        result_tmp_bufs.Add(cur_buf);
+        break;
+      }
+
+      // |avail_buf_size| == 0 case.

[Tom Sepez, 2015/09/23 21:48:41]

ditto

[Lei Zhang, 2015/09/25 09:33:48]

Done.

+      result_tmp_bufs.Add(cur_buf);
+      cur_buf = FX_Alloc(uint8_t, buf_size + 1);
+      cur_buf[buf_size] = '\0';
+    }
+    dest_size = FPDFAPI_FlateGetTotalOut(context);
+    offset = FPDFAPI_FlateGetTotalIn(context);
+    if (result_tmp_bufs.GetSize() == 1) {
+      dest_buf = result_tmp_bufs[0];
+    } else {
+      uint8_t* result_buf = FX_Alloc(uint8_t, dest_size);
+      FX_DWORD result_pos = 0;
+      for (int32_t i = 0; i < result_tmp_bufs.GetSize(); i++) {
+        uint8_t* tmp_buf = result_tmp_bufs[i];
+        FX_DWORD tmp_buf_size = buf_size;
+        if (i == result_tmp_bufs.GetSize() - 1) {
+          tmp_buf_size = last_buf_size;
+        }
+        FXSYS_memcpy(result_buf + result_pos, tmp_buf, tmp_buf_size);
+        result_pos += tmp_buf_size;
+        FX_Free(result_tmp_bufs[i]);
+      }
+      dest_buf = result_buf;
+    }
+  }
+  FPDFAPI_FlateEnd(context);
+}
+
+}  // namespace
+
 class CCodec_FlateScanlineDecoder : public CCodec_ScanlineDecoder {
  public:
   CCodec_FlateScanlineDecoder();
@@ -759,113 +888,7 @@ uint8_t* CCodec_FlateScanlineDecoder::v_GetNextLine() {
 FX_DWORD CCodec_FlateScanlineDecoder::GetSrcOffset() {
   return FPDFAPI_FlateGetTotalIn(m_pFlate);
 }
-static void FlateUncompress(const uint8_t* src_buf,
-                            FX_DWORD src_size,
-                            FX_DWORD orig_size,
-                            uint8_t*& dest_buf,
-                            FX_DWORD& dest_size,
-                            FX_DWORD& offset) {
-  const FX_BOOL useOldImpl = src_size < 10240;
-  FX_DWORD guess_size = orig_size ? orig_size : src_size * 2;
-  FX_DWORD alloc_step =
-      orig_size ? 10240 : (src_size < 10240 ? 10240 : src_size);
-  static const FX_DWORD kMaxInitialAllocSize = 10000000;
-  if (guess_size > kMaxInitialAllocSize) {
-    guess_size = kMaxInitialAllocSize;
-    alloc_step = kMaxInitialAllocSize;
-  }
-  FX_DWORD buf_size = guess_size;
-  FX_DWORD last_buf_size = buf_size;
-  void* context = nullptr;
 
-  uint8_t* guess_buf = FX_Alloc(uint8_t, guess_size + 1);
-  uint8_t* cur_buf = guess_buf;
-  guess_buf[guess_size] = '\0';
-  context = FPDFAPI_FlateInit(my_alloc_func, my_free_func);
-  if (!context)
-    goto fail;
-  FPDFAPI_FlateInput(context, src_buf, src_size);
-  if (useOldImpl) {
-    while (1) {
-      int32_t ret = FPDFAPI_FlateOutput(context, cur_buf, buf_size);
-      if (ret != Z_OK)
-        break;
-      int32_t avail_buf_size = FPDFAPI_FlateGetAvailOut(context);
-      if (avail_buf_size != 0)
-        break;
-
-      // |avail_buf_size| == 0 case.
-      FX_DWORD old_size = guess_size;
-      guess_size += alloc_step;
-      if (guess_size < old_size || guess_size + 1 < guess_size)
-        goto fail;
-      guess_buf = FX_Realloc(uint8_t, guess_buf, guess_size + 1);
-      if (!guess_buf)
-        goto fail;
-      guess_buf[guess_size] = '\0';
-      cur_buf = guess_buf + old_size;
-      buf_size = guess_size - old_size;
-    }
-    dest_size = FPDFAPI_FlateGetTotalOut(context);
-    offset = FPDFAPI_FlateGetTotalIn(context);
-    if (guess_size / 2 > dest_size) {
-      guess_buf = FX_Realloc(uint8_t, guess_buf, dest_size + 1);
-      if (!guess_buf)
-        goto fail;
-      guess_size = dest_size;
-      guess_buf[guess_size] = '\0';
-    }
-    dest_buf = guess_buf;
-  } else {
-    CFX_ArrayTemplate<uint8_t*> result_tmp_bufs;
-    while (1) {
-      int32_t ret = FPDFAPI_FlateOutput(context, cur_buf, buf_size);
-      int32_t avail_buf_size = FPDFAPI_FlateGetAvailOut(context);
-      if (ret != Z_OK) {
-        last_buf_size = buf_size - avail_buf_size;
-        result_tmp_bufs.Add(cur_buf);
-        break;
-      }
-      if (avail_buf_size != 0) {
-        last_buf_size = buf_size - avail_buf_size;
-        result_tmp_bufs.Add(cur_buf);
-        break;
-      }
-
-      // |avail_buf_size| == 0 case.
-      result_tmp_bufs.Add(cur_buf);
-      cur_buf = FX_Alloc(uint8_t, buf_size + 1);
-      cur_buf[buf_size] = '\0';
-    }
-    dest_size = FPDFAPI_FlateGetTotalOut(context);
-    offset = FPDFAPI_FlateGetTotalIn(context);
-    if (result_tmp_bufs.GetSize() == 1) {
-      dest_buf = result_tmp_bufs[0];
-    } else {
-      uint8_t* result_buf = FX_Alloc(uint8_t, dest_size);
-      FX_DWORD result_pos = 0;
-      for (int32_t i = 0; i < result_tmp_bufs.GetSize(); i++) {
-        uint8_t* tmp_buf = result_tmp_bufs[i];
-        FX_DWORD tmp_buf_size = buf_size;
-        if (i == result_tmp_bufs.GetSize() - 1) {
-          tmp_buf_size = last_buf_size;
-        }
-        FXSYS_memcpy(result_buf + result_pos, tmp_buf, tmp_buf_size);
-        result_pos += tmp_buf_size;
-        FX_Free(result_tmp_bufs[i]);
-      }
-      dest_buf = result_buf;
-    }
-  }
-  FPDFAPI_FlateEnd(context);
-  return;
-
-fail:
-  FX_Free(guess_buf);
-  dest_buf = nullptr;
-  dest_size = 0;
-  return;
-}
 ICodec_ScanlineDecoder* CCodec_FlateModule::CreateDecoder(
     const uint8_t* src_buf,
     FX_DWORD src_size,