linux: turn on -Wextra

This is a followup to an earlier change (r38266) which did most of the
warning-related cleanup.  This one just flips the flag, and fixes some
new warnings that crept in during the window while the flag was off.

Second try, now with some libpng fixes.

BUG=34160

Review URL: http://codereview.chromium.org/1320011

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@42700 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 76 insertions, 73 deletions

diff --git a/build/common.gypi b/build/common.gypi
index 6442f40..a4328bb 100644
--- a/build/common.gypi
+++ b/build/common.gypi
@@ -704,8 +704,7 @@
           '-pthread',
           '-fno-exceptions',
           '-Wall',
-          # TODO(evan): turn this back on once the v8 change lands.
-          #'-Wextra',
+          '-Wextra',
           # Don't warn about unused function params.  We use those everywhere.
           '-Wno-unused-parameter',
           # Don't warn about the "struct foo f = {0};" initialization pattern.
diff --git a/chrome/common/extensions/extension.h b/chrome/common/extensions/extension.h
index 05c1522..0129858 100644
--- a/chrome/common/extensions/extension.h
+++ b/chrome/common/extensions/extension.h
@@ -304,7 +304,7 @@ class Extension {
 
   const std::string& launch_local_path() const { return launch_local_path_; }
   const std::string& launch_web_url() const { return launch_web_url_; }
-  const LaunchContainer launch_container() const { return launch_container_; }
+  LaunchContainer launch_container() const { return launch_container_; }
 
   // Gets the fully resolved absolute launch URL.
   GURL GetFullLaunchURL() const;
diff --git a/gfx/codec/png_codec.cc b/gfx/codec/png_codec.cc
index eaf94f0..9db0095 100644
--- a/gfx/codec/png_codec.cc
+++ b/gfx/codec/png_codec.cc
@@ -541,21 +541,53 @@ void ConvertBGRAtoRGB(const unsigned char* bgra, int pixel_width,
   }
 }
 
-// Automatically destroys the given write structs on destruction to make
-// cleanup and error handling code cleaner.
-class PngWriteStructDestroyer {
- public:
-  PngWriteStructDestroyer(png_struct** ps, png_info** pi) : ps_(ps), pi_(pi) {
-  }
-  ~PngWriteStructDestroyer() {
-    png_destroy_write_struct(ps_, pi_);
+// The type of functions usable for converting between pixel formats.
+typedef void (*FormatConverter)(const unsigned char* in, int w,
+                                unsigned char* out, bool* is_opaque);
+
+// libpng uses a wacky setjmp-based API, which makes the compiler nervous.
+// We constrain all of the calls we make to libpng where the setjmp() is in
+// place to this function.
+// Returns true on success.
+bool DoLibpngWrite(png_struct* png_ptr, png_info* info_ptr,
+                   PngEncoderState* state,
+                   int width, int height, int row_byte_width,
+                   const unsigned char* input,
+                   int png_output_color_type, int output_color_components,
+                   FormatConverter converter) {
+  // Make sure to not declare any locals here -- locals in the presence
+  // of setjmp() in C++ code makes gcc complain.
+
+  if (setjmp(png_jmpbuf(png_ptr)))
+    return false;
+
+  // Set our callback for libpng to give us the data.
+  png_set_write_fn(png_ptr, state, EncoderWriteCallback, NULL);
+
+  png_set_IHDR(png_ptr, info_ptr, width, height, 8, png_output_color_type,
+               PNG_INTERLACE_NONE, PNG_COMPRESSION_TYPE_DEFAULT,
+               PNG_FILTER_TYPE_DEFAULT);
+  png_write_info(png_ptr, info_ptr);
+
+  if (!converter) {
+    // No conversion needed, give the data directly to libpng.
+    for (int y = 0; y < height; y ++) {
+      png_write_row(png_ptr,
+                    const_cast<unsigned char*>(&input[y * row_byte_width]));
+    }
+  } else {
+    // Needs conversion using a separate buffer.
+    unsigned char* row = new unsigned char[width * output_color_components];
+    for (int y = 0; y < height; y ++) {
+      converter(&input[y * row_byte_width], width, row, NULL);
+      png_write_row(png_ptr, row);
+    }
+    delete[] row;
   }
- private:
-  png_struct** ps_;
-  png_info** pi_;
 
-  DISALLOW_EVIL_CONSTRUCTORS(PngWriteStructDestroyer);
-};
+  png_write_end(png_ptr, info_ptr);
+  return true;
+}
 
 }  // namespace
 
@@ -566,8 +598,7 @@ bool PNGCodec::Encode(const unsigned char* input, ColorFormat format,
                       std::vector<unsigned char>* output) {
   // Run to convert an input row into the output row format, NULL means no
   // conversion is necessary.
-  void (*converter)(const unsigned char* in, int w, unsigned char* out,
-                    bool* is_opaque) = NULL;
+  FormatConverter converter = NULL;
 
   int input_color_components, output_color_components;
   int png_output_color_type;
@@ -635,42 +666,15 @@ bool PNGCodec::Encode(const unsigned char* input, ColorFormat format,
     png_destroy_write_struct(&png_ptr, NULL);
     return false;
   }
-  PngWriteStructDestroyer destroyer(&png_ptr, &info_ptr);
-
-  if (setjmp(png_jmpbuf(png_ptr))) {
-    // The destroyer will ensure that the structures are cleaned up in this
-    // case, even though we may get here as a jump from random parts of the
-    // PNG library called below.
-    return false;
-  }
 
-  // Set our callback for libpng to give us the data.
   PngEncoderState state(output);
-  png_set_write_fn(png_ptr, &state, EncoderWriteCallback, NULL);
+  bool success = DoLibpngWrite(png_ptr, info_ptr, &state,
+                               w, h, row_byte_width, input,
+                               png_output_color_type, output_color_components,
+                               converter);
+  png_destroy_write_struct(&png_ptr, &info_ptr);
 
-  png_set_IHDR(png_ptr, info_ptr, w, h, 8, png_output_color_type,
-               PNG_INTERLACE_NONE, PNG_COMPRESSION_TYPE_DEFAULT,
-               PNG_FILTER_TYPE_DEFAULT);
-  png_write_info(png_ptr, info_ptr);
-
-  if (!converter) {
-    // No conversion needed, give the data directly to libpng.
-    for (int y = 0; y < h; y ++) {
-      png_write_row(png_ptr,
-                    const_cast<unsigned char*>(&input[y * row_byte_width]));
-    }
-  } else {
-    // Needs conversion using a separate buffer.
-    unsigned char* row = new unsigned char[w * output_color_components];
-    for (int y = 0; y < h; y ++) {
-      converter(&input[y * row_byte_width], w, row, NULL);
-      png_write_row(png_ptr, row);
-    }
-    delete[] row;
-  }
-
-  png_write_end(png_ptr, info_ptr);
-  return true;
+  return success;
 }
 
 // static
diff --git a/sandbox/linux/seccomp/library.cc b/sandbox/linux/seccomp/library.cc
index 1b06bc1..768b00a 100644
--- a/sandbox/linux/seccomp/library.cc
+++ b/sandbox/linux/seccomp/library.cc
@@ -318,7 +318,7 @@ const Elf_Shdr* Library::getSection(const string& section) {
   return &iter->second.second;
 }
 
-const int Library::getSectionIndex(const string& section) {
+int Library::getSectionIndex(const string& section) {
   if (!valid_) {
     return -1;
   }
diff --git a/sandbox/linux/seccomp/library.h b/sandbox/linux/seccomp/library.h
index 96ec581..e27bfde 100644
--- a/sandbox/linux/seccomp/library.h
+++ b/sandbox/linux/seccomp/library.h
@@ -126,7 +126,7 @@ class Library {
   bool parseElf();
   const Elf_Ehdr* getEhdr();
   const Elf_Shdr* getSection(const string& section);
-  const int getSectionIndex(const string& section);
+  int getSectionIndex(const string& section);
   void makeWritable(bool state) const;
   void patchSystemCalls();
   bool isVDSO() const { return isVDSO_; }
diff --git a/sandbox/linux/seccomp/sandbox_impl.h b/sandbox/linux/seccomp/sandbox_impl.h
index 0a98283..18a359c 100644
--- a/sandbox/linux/seccomp/sandbox_impl.h
+++ b/sandbox/linux/seccomp/sandbox_impl.h
@@ -268,7 +268,7 @@ class Sandbox {
   // Wrapper around "read()" that can deal with partial and interrupted reads
   // and that does not modify the global errno variable.
   static ssize_t read(SysCalls& sys, int fd, void* buf, size_t len) {
-    if (len < 0) {
+    if (static_cast<ssize_t>(len) < 0) {
       sys.my_errno = EINVAL;
       return -1;
     }
diff --git a/sandbox/linux/seccomp/socketcall.cc b/sandbox/linux/seccomp/socketcall.cc
index 43116bb..497e5e2 100644
--- a/sandbox/linux/seccomp/socketcall.cc
+++ b/sandbox/linux/seccomp/socketcall.cc
@@ -288,8 +288,7 @@ bool Sandbox::process_sendmsg(int parentMapsFd, int sandboxFd, int threadFdPub,
     die("Failed to read parameters for sendmsg() [process]");
   }
 
-  if (data.msg.msg_namelen    < 0 || data.msg.msg_namelen    > 4096 ||
-      data.msg.msg_controllen < 0 || data.msg.msg_controllen > 4096) {
+  if (data.msg.msg_namelen > 4096 || data.msg.msg_controllen > 4096) {
     die("Unexpected size for socketcall() payload [process]");
   }
   char extra[data.msg.msg_namelen + data.msg.msg_controllen];
@@ -767,7 +766,7 @@ bool Sandbox::process_socketcall(int parentMapsFd, int sandboxFd,
   // Verify that the length for the payload is reasonable. We don't want to
   // blow up our stack, and excessive (or negative) buffer sizes are almost
   // certainly a bug.
-  if (numExtraData < 0 || numExtraData > 4096) {
+  if (numExtraData > 4096) {
     die("Unexpected size for socketcall() payload [process]");
   }
 
@@ -783,8 +782,7 @@ bool Sandbox::process_socketcall(int parentMapsFd, int sandboxFd,
   ssize_t numSendmsgExtra = 0;
   if (socketcall_req.call == SYS_SENDMSG) {
     struct msghdr* msg = reinterpret_cast<struct msghdr*>(extra);
-    if (msg->msg_namelen    < 0 || msg->msg_namelen    > 4096 ||
-        msg->msg_controllen < 0 || msg->msg_controllen > 4096) {
+    if (msg->msg_namelen > 4096 || msg->msg_controllen > 4096) {
       die("Unexpected size for socketcall() payload [process]");
     }
     numSendmsgExtra = msg->msg_namelen + msg->msg_controllen;
diff --git a/sandbox/linux/suid/linux_util.c b/sandbox/linux/suid/linux_util.c
index ded545b..c5af0d0 100644
--- a/sandbox/linux/suid/linux_util.c
+++ b/sandbox/linux/suid/linux_util.c
@@ -83,8 +83,9 @@ bool FindProcessHoldingSocket(pid_t* pid_out, ino_t socket_inode) {
       continue;
 
     while ((dent = readdir(fd))) {
-      if (snprintf(buf, sizeof(buf), "/proc/%lu/fd/%s", pid_ul,
-                   dent->d_name) >= sizeof(buf) - 1) {
+      int printed = snprintf(buf, sizeof(buf), "/proc/%lu/fd/%s", pid_ul,
+                             dent->d_name);
+      if (printed < 0 || printed >= (int)(sizeof(buf) - 1)) {
         continue;
       }
 
diff --git a/sandbox/linux/suid/process_util_linux.c b/sandbox/linux/suid/process_util_linux.c
index 9f40f39..7b31274 100644
--- a/sandbox/linux/suid/process_util_linux.c
+++ b/sandbox/linux/suid/process_util_linux.c
@@ -38,7 +38,7 @@ bool AdjustOOMScore(pid_t process, int score) {
 
   char buf[3];  // 0 <= |score| <= 15;
   snprintf(buf, sizeof(buf), "%d", score);
-  size_t len = strlen(buf);
+  ssize_t len = strlen(buf);
 
   ssize_t bytes_written = write(fd, buf, len);
   close(fd);
diff --git a/sandbox/linux/suid/sandbox.c b/sandbox/linux/suid/sandbox.c
index e4968c9..7389f03 100644
--- a/sandbox/linux/suid/sandbox.c
+++ b/sandbox/linux/suid/sandbox.c
@@ -90,13 +90,13 @@ static int CloneChrootHelperProcess() {
   char tempDirectoryTemplate[80] = "/tmp/chrome-sandbox-chroot-XXXXXX";
   struct statfs sfs;
   if (!statfs("/tmp", &sfs) &&
-      sfs.f_type != BTRFS_SUPER_MAGIC &&
-      sfs.f_type != EXT2_SUPER_MAGIC &&
-      sfs.f_type != EXT3_SUPER_MAGIC &&
-      sfs.f_type != EXT4_SUPER_MAGIC &&
-      sfs.f_type != REISERFS_SUPER_MAGIC &&
-      sfs.f_type != TMPFS_MAGIC &&
-      sfs.f_type != XFS_SUPER_MAGIC) {
+      (unsigned long)sfs.f_type != BTRFS_SUPER_MAGIC &&
+      (unsigned long)sfs.f_type != EXT2_SUPER_MAGIC &&
+      (unsigned long)sfs.f_type != EXT3_SUPER_MAGIC &&
+      (unsigned long)sfs.f_type != EXT4_SUPER_MAGIC &&
+      (unsigned long)sfs.f_type != REISERFS_SUPER_MAGIC &&
+      (unsigned long)sfs.f_type != TMPFS_MAGIC &&
+      (unsigned long)sfs.f_type != XFS_SUPER_MAGIC) {
     // If /dev/shm exists, it is supposed to be a tmpfs filesystem. While we
     // are not actually using it for shared memory, moving our temp directory
     // into a known tmpfs filesystem is preferable over using a potentially
@@ -137,7 +137,7 @@ static int CloneChrootHelperProcess() {
   char proc_self_fd_str[128];
   int printed = snprintf(proc_self_fd_str, sizeof(proc_self_fd_str),
                          "/proc/self/fd/%d", chroot_dir_fd);
-  if (printed < 0 || printed >= sizeof(proc_self_fd_str)) {
+  if (printed < 0 || printed >= (int)sizeof(proc_self_fd_str)) {
     fprintf(stderr, "Error in snprintf");
     return -1;
   }
@@ -246,7 +246,7 @@ static bool SpawnChrootHelper() {
   // number of the file descriptor.
   char desc_str[64];
   int printed = snprintf(desc_str, sizeof(desc_str), "%d", chroot_signal_fd);
-  if (printed < 0 || printed >= sizeof(desc_str)) {
+  if (printed < 0 || printed >= (int)sizeof(desc_str)) {
     fprintf(stderr, "Failed to snprintf\n");
     return false;
   }
@@ -379,9 +379,10 @@ int main(int argc, char **argv) {
   if (argc == 4 && (0 == strcmp(argv[1], kAdjustOOMScoreSwitch))) {
     char* endptr;
     long score;
-    pid_t pid = strtoul(argv[2], &endptr, 10);
-    if (pid == ULONG_MAX || *endptr)
+    unsigned long pid_ul = strtoul(argv[2], &endptr, 10);
+    if (pid_ul == ULONG_MAX || *endptr)
       return 1;
+    pid_t pid = pid_ul;
     score = strtol(argv[3], &endptr, 10);
     if (score == LONG_MAX || score == LONG_MIN || *endptr)
       return 1;