diff mbox series

debuginfod: Check result of calling MHD_add_response_header.

Message ID 20211201113847.15352-1-mark@klomp.org
State Superseded
Headers show
Series debuginfod: Check result of calling MHD_add_response_header. | expand

Commit Message

Mark Wielaard Dec. 1, 2021, 11:38 a.m. UTC
Although unlikely the MHD_add_response_header can fail for
various reasons.  If it fails something odd is going on.
So check we can actually add a response header before using
the response.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 debuginfod/ChangeLog      |  10 +++
 debuginfod/debuginfod.cxx | 127 ++++++++++++++++++++++++++++----------
 2 files changed, 105 insertions(+), 32 deletions(-)

Comments

Frank Ch. Eigler Dec. 1, 2021, 12:02 p.m. UTC | #1
Hi -

> Although unlikely the MHD_add_response_header can fail for
> various reasons.  If it fails something odd is going on.
> So check we can actually add a response header before using
> the response.

ISTM it is okay to send the response object (the body), even if
something goes wrong with adding optional headers later.


- FChE
Mark Wielaard Dec. 1, 2021, 12:24 p.m. UTC | #2
H Frank,

On Wed, 2021-12-01 at 07:02 -0500, Frank Ch. Eigler wrote:
> Although unlikely the MHD_add_response_header can fail for
> > various reasons.  If it fails something odd is going on.
> > So check we can actually add a response header before using
> > the response.
> 
> ISTM it is okay to send the response object (the body), even if
> something goes wrong with adding optional headers later.

OK, so log error, but proceed sending the response with the data
already added. I'll update my patch to do that instead. I don't really
think this will really fail often (or at all). But if it does, it would
be good to be able to catch it.

Thanks,

Mark
diff mbox series

Patch

diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 822bd637..7054fdad 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,13 @@ 
+2021-12-01  Mark Wielaard  <mark@klomp.org>
+
+	* debuginfod.cxx (reportable_exception::mhd_send_response): Check
+	MHD_add_response_header result.
+	(add_mhd_last_modified): Likewise and return true when successful.
+	(handle_buildid_f_match): Check MHD_add_response_header result.
+	(handle_buildid_r_match): Likewise.
+	(handle_metrics): Likewise.
+	(handle_root): Likewise.
+
 2021-11-10  √Črico N. Rolim  <erico.erc@gmail.com>
 
 	* debuginfod.cxx: include "system.h" under 'extern "C"' block.
diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index 764e7b94..3b4591dd 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -1,5 +1,6 @@ 
 /* Debuginfo-over-http server.
    Copyright (C) 2019-2021 Red Hat, Inc.
+   Copyright (C) 2021 Mark J. Wielaard <mark@klomp.org>
    This file is part of elfutils.
 
    This file is free software; you can redistribute it and/or modify
@@ -646,8 +647,10 @@  struct reportable_exception
     MHD_Response* r = MHD_create_response_from_buffer (message.size(),
                                                        (void*) message.c_str(),
                                                        MHD_RESPMEM_MUST_COPY);
-    MHD_add_response_header (r, "Content-Type", "text/plain");
-    MHD_RESULT rc = MHD_queue_response (c, code, r);
+    MHD_RESULT rc;
+    rc = MHD_add_response_header (r, "Content-Type", "text/plain");
+    if (rc != MHD_NO)
+      rc = MHD_queue_response (c, code, r);
     MHD_destroy_response (r);
     return rc;
   }
@@ -1067,7 +1070,10 @@  conninfo (struct MHD_Connection * conn)
 ////////////////////////////////////////////////////////////////////////
 
 
-static void
+/* Adds a Last-Modified and Cache-Control header to the response.
+   Returns true when headers could be added, false when an error
+   occured adding an header.  */
+static bool
 add_mhd_last_modified (struct MHD_Response *resp, time_t mtime)
 {
   struct tm *now = gmtime (&mtime);
@@ -1076,10 +1082,14 @@  add_mhd_last_modified (struct MHD_Response *resp, time_t mtime)
       char datebuf[80];
       size_t rc = strftime (datebuf, sizeof (datebuf), "%a, %d %b %Y %T GMT", now);
       if (rc > 0 && rc < sizeof (datebuf))
-        (void) MHD_add_response_header (resp, "Last-Modified", datebuf);
+        if (MHD_add_response_header (resp, "Last-Modified", datebuf) == MHD_NO)
+	  return false;
     }
 
-  (void) MHD_add_response_header (resp, "Cache-Control", "public");
+  if (MHD_add_response_header (resp, "Cache-Control", "public") == MHD_NO)
+    return false;
+
+  return true;
 }
 
 
@@ -1125,11 +1135,22 @@  handle_buildid_f_match (bool internal_req_t,
     }
   else
     {
-      MHD_add_response_header (r, "Content-Type", "application/octet-stream");
       std::string file = b_source0.substr(b_source0.find_last_of("/")+1, b_source0.length());
-      MHD_add_response_header (r, "X-DEBUGINFOD-SIZE", to_string(s.st_size).c_str() );
-      MHD_add_response_header (r, "X-DEBUGINFOD-FILE", file.c_str() );
-      add_mhd_last_modified (r, s.st_mtime);
+      if (MHD_add_response_header (r, "Content-Type",
+				   "application/octet-stream") == MHD_NO
+	  || MHD_add_response_header (r, "X-DEBUGINFOD-SIZE",
+				      to_string(s.st_size).c_str()) == MHD_NO
+	  || MHD_add_response_header (r, "X-DEBUGINFOD-FILE",
+				      file.c_str()) == MHD_NO
+	  || add_mhd_last_modified (r, s.st_mtime) == false)
+	{
+	  if (verbose)
+	    obatched(clog) << "cannot add response headers for "
+			   << b_source0 << endl;
+	  close (fd);
+	  MHD_destroy_response (r);
+	  return NULL;
+	}
       if (verbose > 1)
         obatched(clog) << "serving file " << b_source0 << endl;
       /* libmicrohttpd will close it. */
@@ -1595,13 +1616,25 @@  handle_buildid_r_match (bool internal_req_p,
           break; // branch out of if "loop", to try new libarchive fetch attempt
         }
 
+      if (MHD_add_response_header (r, "Content-Type",
+				   "application/octet-stream") == MHD_NO
+	  || MHD_add_response_header (r, "X-DEBUGINFOD-SIZE",
+				      to_string(fs.st_size).c_str()) == MHD_NO
+	  || MHD_add_response_header (r, "X-DEBUGINFOD-ARCHIVE",
+				      b_source0.c_str()) == MHD_NO
+	  || MHD_add_response_header (r, "X-DEBUGINFOD-FILE",
+				      b_source1.c_str()) == MHD_NO
+	  || add_mhd_last_modified (r, fs.st_mtime))
+	{
+          if (verbose)
+            obatched(clog) << "cannot add response header for "
+			   << b_source0 << endl;
+	  close (fd);
+	  break; // branch out of if "loop", try new libarchive fetch attempt
+	}
+
       inc_metric ("http_responses_total","result","archive fdcache");
 
-      MHD_add_response_header (r, "Content-Type", "application/octet-stream");
-      MHD_add_response_header (r, "X-DEBUGINFOD-SIZE", to_string(fs.st_size).c_str());
-      MHD_add_response_header (r, "X-DEBUGINFOD-ARCHIVE", b_source0.c_str());
-      MHD_add_response_header (r, "X-DEBUGINFOD-FILE", b_source1.c_str());
-      add_mhd_last_modified (r, fs.st_mtime);
       if (verbose > 1)
         obatched(clog) << "serving fdcache archive " << b_source0 << " file " << b_source1 << endl;
       /* libmicrohttpd will close it. */
@@ -1741,13 +1774,25 @@  handle_buildid_r_match (bool internal_req_p,
         }
       else
         {
-          MHD_add_response_header (r, "Content-Type", "application/octet-stream");
           std::string file = b_source1.substr(b_source1.find_last_of("/")+1, b_source1.length());
-          MHD_add_response_header (r, "X-DEBUGINFOD-SIZE", to_string(fs.st_size).c_str());
-          MHD_add_response_header (r, "X-DEBUGINFOD-ARCHIVE", b_source0.c_str());
-          MHD_add_response_header (r, "X-DEBUGINFOD-FILE", file.c_str());
-
-          add_mhd_last_modified (r, archive_entry_mtime(e));
+          if (MHD_add_response_header (r, "Content-Type",
+				       "application/octet-stream") == MHD_NO
+	      || MHD_add_response_header (r, "X-DEBUGINFOD-SIZE",
+					  to_string(fs.st_size).c_str()) == MHD_NO
+	      || MHD_add_response_header (r, "X-DEBUGINFOD-ARCHIVE",
+					  b_source0.c_str()) == MHD_NO
+	      || MHD_add_response_header (r, "X-DEBUGINFOD-FILE",
+					  file.c_str()) == MHD_NO
+	      || add_mhd_last_modified (r, archive_entry_mtime(e)) == false)
+	    {
+	      if (verbose)
+		obatched(clog) << "cannot create response header for "
+			       << b_source0 << endl;
+	      close(fd);
+	      MHD_destroy_response (r);
+	      r = NULL;
+	      break; // assume no chance of better luck around another iteration; no other copies of same file
+	    }
           if (verbose > 1)
             obatched(clog) << "serving archive " << b_source0 << " file " << b_source1 << endl;
           /* libmicrohttpd will close it. */
@@ -2012,14 +2057,18 @@  and will not query the upstream servers");
           auto r = MHD_create_response_from_fd ((uint64_t) s.st_size, fd);
           if (r)
             {
-              MHD_add_response_header (r, "Content-Type", "application/octet-stream");
-              add_mhd_last_modified (r, s.st_mtime);
-              if (verbose > 1)
-                obatched(clog) << "serving file from upstream debuginfod/cache" << endl;
-              if (result_fd)
-                *result_fd = fd;
-              return r; // NB: don't close fd; libmicrohttpd will
-            }
+              if (MHD_add_response_header (r, "Content-Type",
+					   "application/octet-stream") != MHD_NO
+		  && add_mhd_last_modified (r, s.st_mtime) != false)
+		{
+		  if (verbose > 1)
+		    obatched(clog) << "serving file from upstream debuginfod/cache" << endl;
+		  if (result_fd)
+		    *result_fd = fd;
+		  return r; // NB: don't close fd; libmicrohttpd will
+		}
+	      MHD_destroy_response (r);
+	    }
         }
       close (fd);
     }
@@ -2163,8 +2212,15 @@  handle_metrics (off_t* size)
   MHD_Response* r = MHD_create_response_from_buffer (os.size(),
                                                      (void*) os.c_str(),
                                                      MHD_RESPMEM_MUST_COPY);
-  *size = os.size();
-  MHD_add_response_header (r, "Content-Type", "text/plain");
+  if (r != NULL)
+    {
+      *size = os.size();
+      if (MHD_add_response_header (r, "Content-Type", "text/plain") == MHD_NO)
+	{
+	  MHD_destroy_response (r);
+	  r = NULL;
+	}
+    }
   return r;
 }
 
@@ -2176,8 +2232,15 @@  handle_root (off_t* size)
   MHD_Response* r = MHD_create_response_from_buffer (version.size (),
 						     (void *) version.c_str (),
 						     MHD_RESPMEM_PERSISTENT);
-  *size = version.size ();
-  MHD_add_response_header (r, "Content-Type", "text/plain");
+  if (r != NULL)
+    {
+      *size = version.size ();
+      if (MHD_add_response_header (r, "Content-Type", "text/plain") == MHD_NO)
+	{
+	  MHD_destroy_response (r);
+	  r = NULL;
+	}
+    }
   return r;
 }