pe(coff): COFF symbols and COFF strings are deprecated

Previously, I (we?) thought that COFF symbols/strings were always there.

In fact, this is not the case and they are deprecated
according to https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#coff-file-header-object-and-image

> The file offset of the COFF symbol table, or zero
> if no COFF symbol table is present. This value should be zero for an image
> because COFF debugging information is deprecated.

src/pe/header.rs

@@ -262,14 +262,24 @@ impl CoffHeader {
     }
 
     /// Return the COFF symbol table.
-    pub fn symbols<'a>(&self, bytes: &'a [u8]) -> error::Result<symbol::SymbolTable<'a>> {
+    pub fn symbols<'a>(&self, bytes: &'a [u8]) -> error::Result<Option<symbol::SymbolTable<'a>>> {
         let offset = self.pointer_to_symbol_table as usize;
         let number = self.number_of_symbol_table as usize;
-        symbol::SymbolTable::parse(bytes, offset, number)
+        if offset == 0 {
+            Ok(None)
+        } else {
+            symbol::SymbolTable::parse(bytes, offset, number).map(Some)
+        }
     }
 
     /// Return the COFF string table.
-    pub fn strings<'a>(&self, bytes: &'a [u8]) -> error::Result<strtab::Strtab<'a>> {
+    pub fn strings<'a>(&self, bytes: &'a [u8]) -> error::Result<Option<strtab::Strtab<'a>>> {
+        // > The file offset of the COFF symbol table, or zero if no COFF symbol table is present.
+        // > This value should be zero for an image because COFF debugging information is deprecated.
+        if self.pointer_to_symbol_table == 0 {
+            return Ok(None);
+        }
+
         let mut offset = self.pointer_to_symbol_table as usize
             + symbol::SymbolTable::size(self.number_of_symbol_table as usize);
 
@@ -279,7 +289,7 @@ impl CoffHeader {
         // The offset needs to be advanced in order to read the strings.
         offset += length_field_size;
 
-        Ok(strtab::Strtab::parse(bytes, offset, length, 0)?)
+        Ok(Some(strtab::Strtab::parse(bytes, offset, length, 0)?))
     }
 }
 

src/pe/mod.rs

@@ -358,70 +358,26 @@ impl<'a> PE<'a> {
                     written_data_size);
             }
         }
-        // COFF Symbol Table
-        // Auxiliary Symbol Records
-        // COFF String Table
-        {
-            // Jump offset to COFF Symbol Table, so we can jump to String Table.
-            let coff_sym_offset = usize::try_from(self.header.coff_header.pointer_to_symbol_table)
-                .map_err(|_| {
-                    Self::Error::Malformed(format!(
-                        "COFF Header's symbol table offset does not fit in platform `usize`"
-                    ))
-                })?;
-            offset = coff_sym_offset;
-            bytes.gwrite_with(
-                self.header.coff_header.symbols(self.bytes)?,
-                &mut offset,
-                ctx,
-            )?;
-            max_offset = max(offset, max_offset);
-            // TODO: empty string table?!
-            //            let coff_strings = self.header.coff_header.strings(self.bytes)?;
-
-            // FIXME: this can truncate a wrong length
-            // should we catch this earlier in the signature of `len` directly?
-            //written += bytes.gwrite_with(coff_strings.len() as u32, &mut offset, ctx)?;
-            //written += bytes.gwrite(coff_strings.bytes, &mut offset)?;
-        }
-        if let Some(opt_header) = self.header.optional_header {
-            // Takes care of:
-            // - export table (.edata)
-            // - import table (.idata)
-            // - bound import table
-            // - import address table
-            // - delay import tables
-            // - resource table (.rsrc)
-            // - exception table (.pdata)
-            // - base relocation table (.reloc)
-            // - debug table (.debug)
-            // - load config table
-            // - tls table (.tls)
-            // - architecture (reserved, 0 for now)
-            // - global ptr is a "empty" data directory (header-only)
-            // - clr runtime header (.cormeta is object-only)
-            for (dt_type, dd) in opt_header.data_directories.dirs() {
-                // - attribute certificate table is a bit special
-                // its size is not the real size of all certificates
-                // you can check the parse logic to understand how that works
-                if dt_type == DataDirectoryType::CertificateTable {
-                    let mut certificate_start = dd.virtual_address.try_into()?;
-                    for certificate in &self.certificates {
-                        bytes.gwrite_with(certificate, &mut certificate_start, ctx)?;
-                        max_offset = max(max_offset, certificate_start);
-                    }
-                } else {
-                    let offset = dd.offset.ok_or(Self::Error::Malformed(
-                        "Data directory was not read with offset information, cannot rewrite"
-                            .into(),
-                    ))?;
-                    let dd_written = bytes.pwrite(dd.data(&self.bytes)?, offset)?;
-                    max_offset = max(max_offset, offset);
-                    debug!(
-                        "writing {:?} at {} for {} bytes",
-                        dt_type, offset, dd_written
-                    );
-                }
+
+        Ok(*offset)
+    }
+
+    pub fn write_certificates(
+        &self,
+        bytes: &mut [u8],
+        ctx: scroll::Endian,
+    ) -> Result<usize, error::Error> {
+        let opt_header = self.header.optional_header.ok_or(error::Error::Malformed(
+            "This PE binary has no optional header; it is required to write certificates"
+                .to_string(),
+        ))?;
+        let mut max_offset = 0;
+
+        if let Some(certificate_directory) = opt_header.data_directories.get_certificate_table() {
+            let mut certificate_start = certificate_directory.virtual_address.try_into()?;
+            for certificate in &self.certificates {
+                bytes.gwrite_with(certificate, &mut certificate_start, ctx)?;
+                max_offset = max(certificate_start, max_offset);
             }
         }
 
@@ -517,10 +473,12 @@ pub struct Coff<'a> {
     pub header: header::CoffHeader,
     /// A list of the sections in this COFF binary
     pub sections: Vec<section_table::SectionTable>,
-    /// The COFF symbol table.
-    pub symbols: symbol::SymbolTable<'a>,
-    /// The string table.
-    pub strings: strtab::Strtab<'a>,
+    /// The COFF symbol table, they are not guaranteed to exist.
+    /// For an image, this is expected to be None as COFF debugging information
+    /// has been deprecated.
+    pub symbols: Option<symbol::SymbolTable<'a>>,
+    /// The string table, they don't exist if COFF symbol table does not exist.
+    pub strings: Option<strtab::Strtab<'a>>,
 }
 
 impl<'a> Coff<'a> {
@@ -657,17 +615,18 @@ mod tests {
     #[test]
     fn string_table_excludes_length() {
         let coff = Coff::parse(&&COFF_FILE_SINGLE_STRING_IN_STRING_TABLE[..]).unwrap();
-        let string_table = coff.strings.to_vec().unwrap();
+        let string_table = coff.strings.unwrap().to_vec().unwrap();
 
         assert!(string_table == vec!["ExitProcess"]);
     }
 
     #[test]
     fn symbol_name_excludes_length() {
         let coff = Coff::parse(&COFF_FILE_SINGLE_STRING_IN_STRING_TABLE).unwrap();
-        let strings = coff.strings;
+        let strings = coff.strings.unwrap();
         let symbols = coff
             .symbols
+            .unwrap()
             .iter()
             .filter(|(_, name, _)| name.is_none())
             .map(|(_, _, sym)| sym.name(&strings).unwrap().to_owned())

src/strtab.rs

@@ -15,7 +15,7 @@ if_alloc! {
 /// with your choice of delimiter. Please be careful.
 pub struct Strtab<'a> {
     delim: ctx::StrCtx,
-    pub bytes: &'a [u8],
+    bytes: &'a [u8],
     #[cfg(feature = "alloc")]
     strings: Vec<(usize, &'a str)>,
 }