interest: prevent InterestLifetime overflow

Refs: #4997 Change-Id: Ic116e4d6d681ab89c655774a90d9d652db4a8916
named-data · Feb 13, 2024 · 7b540d2 · 7b540d2
1 parent 8e047e1
commit 7b540d2
Show file tree

Hide file tree

Showing 7 changed files with 125 additions and 62 deletions.
diff --git a/ndn-cxx/data.hpp b/ndn-cxx/data.hpp
@@ -44,19 +44,23 @@ class Data : public PacketBase, public std::enable_shared_from_this<Data>
     using tlv::Error::Error;
   };
 
-  /** @brief Construct an unsigned Data packet with given @p name and empty Content.
-   *  @warning In certain contexts that use `Data::shared_from_this()`, Data must be created using
-   *           `std::make_shared`. Otherwise, `shared_from_this()` may trigger undefined behavior.
-   *           One example where this is necessary is storing Data into a subclass of InMemoryStorage.
+  /**
+   * @brief Construct an unsigned Data packet with given @p name and empty Content.
+   *
+   * @warning In certain contexts that use `Data::shared_from_this()`, the Data must be created
+   *          using `std::make_shared`. Otherwise, `shared_from_this()` will throw `std::bad_weak_ptr`.
+   *          One example where this is necessary is storing Data into a subclass of InMemoryStorage.
    */
   explicit
   Data(const Name& name = Name());
 
-  /** @brief Construct a Data packet by decoding from @p wire.
-   *  @param wire TLV element of type tlv::Data; may be signed or unsigned.
-   *  @warning In certain contexts that use `Data::shared_from_this()`, Data must be created using
-   *           `std::make_shared`. Otherwise, `shared_from_this()` may trigger undefined behavior.
-   *           One example where this is necessary is storing Data into a subclass of InMemoryStorage.
+  /**
+   * @brief Construct a Data packet by decoding from @p wire.
+   * @param wire TLV element of type tlv::Data; may be signed or unsigned.
+   *
+   * @warning In certain contexts that use `Data::shared_from_this()`, the Data must be created
+   *          using `std::make_shared`. Otherwise, `shared_from_this()` will throw `std::bad_weak_ptr`.
+   *          One example where this is necessary is storing Data into a subclass of InMemoryStorage.
    */
   explicit
   Data(const Block& wire);
@@ -301,37 +305,54 @@ class Data : public PacketBase, public std::enable_shared_from_this<Data>
   extractSignedRanges() const;
 
 public: // MetaInfo fields
+  /**
+   * @copydoc MetaInfo::getType()
+   */
   uint32_t
-  getContentType() const
+  getContentType() const noexcept
   {
     return m_metaInfo.getType();
   }
 
+  /**
+   * @copydoc MetaInfo::setType()
+   */
   Data&
   setContentType(uint32_t type);
 
+  /**
+   * @copydoc MetaInfo::getFreshnessPeriod()
+   */
   time::milliseconds
-  getFreshnessPeriod() const
+  getFreshnessPeriod() const noexcept
   {
     return m_metaInfo.getFreshnessPeriod();
   }
 
+  /**
+   * @copydoc MetaInfo::setFreshnessPeriod()
+   */
   Data&
   setFreshnessPeriod(time::milliseconds freshnessPeriod);
 
+  /**
+   * @copydoc MetaInfo::getFinalBlock()
+   */
   const std::optional<name::Component>&
-  getFinalBlock() const
+  getFinalBlock() const noexcept
   {
     return m_metaInfo.getFinalBlock();
   }
 
+  /**
+   * @copydoc MetaInfo::setFinalBlock()
+   */
   Data&
   setFinalBlock(std::optional<name::Component> finalBlockId);
 
 public: // SignatureInfo fields
   /**
-   * @brief Get the `SignatureType`.
-   * @return tlv::SignatureTypeValue, or -1 to indicate the signature is invalid.
+   * @copydoc SignatureInfo::getSignatureType()
    */
   int32_t
   getSignatureType() const noexcept

diff --git a/ndn-cxx/interest.cpp b/ndn-cxx/interest.cpp
@@ -1,6 +1,6 @@
 /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
 /*
- * Copyright (c) 2013-2023 Regents of the University of California.
+ * Copyright (c) 2013-2024 Regents of the University of California.
  *
  * This file is part of ndn-cxx library (NDN C++ library with eXperimental eXtensions).
  *
@@ -82,14 +82,13 @@ Interest::wireEncode(EncodingImpl<TAG>& encoder) const
   }
 
   // HopLimit
-  if (getHopLimit()) {
+  if (m_hopLimit) {
     totalLength += prependBinaryBlock(encoder, tlv::HopLimit, {*m_hopLimit});
   }
 
   // InterestLifetime
-  if (getInterestLifetime() != DEFAULT_INTEREST_LIFETIME) {
-    totalLength += prependNonNegativeIntegerBlock(encoder, tlv::InterestLifetime,
-                                                  static_cast<uint64_t>(getInterestLifetime().count()));
+  if (m_interestLifetime != DEFAULT_INTEREST_LIFETIME.count()) {
+    totalLength += prependNonNegativeIntegerBlock(encoder, tlv::InterestLifetime, m_interestLifetime);
   }
 
   // Nonce
@@ -177,7 +176,7 @@ Interest::wireDecode(const Block& wire)
   m_canBePrefix = m_mustBeFresh = false;
   m_forwardingHint.clear();
   m_nonce.reset();
-  m_interestLifetime = DEFAULT_INTEREST_LIFETIME;
+  m_interestLifetime = DEFAULT_INTEREST_LIFETIME.count();
   m_hopLimit.reset();
   m_parameters.clear();
 
@@ -262,7 +261,7 @@ Interest::wireDecode(const Block& wire)
         if (lastElement >= 6) {
           NDN_THROW(Error("InterestLifetime element is out of order"));
         }
-        m_interestLifetime = time::milliseconds(readNonNegativeInteger(*element));
+        m_interestLifetime = readNonNegativeInteger(*element);
         lastElement = 6;
         break;
       }
@@ -439,15 +438,25 @@ Interest::refreshNonce()
   m_wire.reset();
 }
 
+time::milliseconds
+Interest::getInterestLifetime() const noexcept
+{
+  if (m_interestLifetime > static_cast<uint64_t>(time::milliseconds::max().count())) {
+    return time::milliseconds::max();
+  }
+  return time::milliseconds(m_interestLifetime);
+}
+
 Interest&
 Interest::setInterestLifetime(time::milliseconds lifetime)
 {
   if (lifetime < 0_ms) {
     NDN_THROW(std::invalid_argument("InterestLifetime must be >= 0"));
   }
 
-  if (lifetime != m_interestLifetime) {
-    m_interestLifetime = lifetime;
+  uint64_t lifetimeMillis = static_cast<uint64_t>(lifetime.count());
+  if (lifetimeMillis != m_interestLifetime) {
+    m_interestLifetime = lifetimeMillis;
     m_wire.reset();
   }
   return *this;

diff --git a/ndn-cxx/interest.hpp b/ndn-cxx/interest.hpp
@@ -94,53 +94,61 @@ class Interest : public PacketBase, public std::enable_shared_from_this<Interest
     }
   };
 
-  /** @brief Construct an Interest with given @p name and @p lifetime.
+  /**
+   * @brief Construct an Interest with given @p name and @p lifetime.
    *
-   *  @throw std::invalid_argument @p name is invalid or @p lifetime is negative
-   *  @warning In certain contexts that use `Interest::shared_from_this()`, Interest must be created
-   *           using `make_shared`. Otherwise, `shared_from_this()` will trigger undefined behavior.
+   * @throw std::invalid_argument @p name is invalid or @p lifetime is negative.
+   * @warning In certain contexts that use `Interest::shared_from_this()`, the Interest must be created
+   *          using `std::make_shared`. Otherwise, `shared_from_this()` will throw `std::bad_weak_ptr`.
    */
   explicit
   Interest(const Name& name = {}, time::milliseconds lifetime = DEFAULT_INTEREST_LIFETIME);
 
-  /** @brief Construct an Interest by decoding from @p wire.
+  /**
+   * @brief Construct an Interest by decoding from @p wire.
    *
-   *  @warning In certain contexts that use `Interest::shared_from_this()`, Interest must be created
-   *           using `make_shared`. Otherwise, `shared_from_this()` will trigger undefined behavior.
+   * @warning In certain contexts that use `Interest::shared_from_this()`, the Interest must be created
+   *          using `std::make_shared`. Otherwise, `shared_from_this()` will throw `std::bad_weak_ptr`.
    */
   explicit
   Interest(const Block& wire);
 
-  /** @brief Prepend wire encoding to @p encoder.
+  /**
+   * @brief Prepend wire encoding to @p encoder.
    */
   template<encoding::Tag TAG>
   size_t
   wireEncode(EncodingImpl<TAG>& encoder) const;
 
-  /** @brief Encode into a Block.
+  /**
+   * @brief Encode into a Block.
    */
   const Block&
   wireEncode() const;
 
-  /** @brief Decode from @p wire.
+  /**
+   * @brief Decode from @p wire.
    */
   void
   wireDecode(const Block& wire);
 
-  /** @brief Check if this instance has cached wire encoding.
+  /**
+   * @brief Check if this instance has cached wire encoding.
    */
   bool
   hasWire() const noexcept
   {
     return m_wire.hasWire();
   }
 
-  /** @brief Return a URI-like string that represents the Interest.
+  /**
+   * @brief Return a URI-like string that represents the Interest.
+   *
+   * The string always starts with the Interest's name in URI format. After the name, if any
+   * of the Interest's CanBePrefix, MustBeFresh, Nonce, InterestLifetime, or HopLimit fields
+   * are present, their textual representation is appended as a query string.
    *
-   *  The string always starts with `getName().toUri()`. After the name, if any of the
-   *  Interest's CanBePrefix, MustBeFresh, Nonce, InterestLifetime, or HopLimit fields
-   *  are present, their textual representation is appended as a query string.
-   *  Example: "/test/name?MustBeFresh&Nonce=123456"
+   * Example: `"/test/name?MustBeFresh&Nonce=123456"`
    */
   std::string
   toUri() const;
@@ -265,12 +273,13 @@ class Interest : public PacketBase, public std::enable_shared_from_this<Interest
 
   /**
    * @brief Get the %Interest's lifetime.
+   *
+   * If the `InterestLifetime` element is not present, returns #DEFAULT_INTEREST_LIFETIME.
+   * If the `InterestLifetime` value is not representable in the return type, it's clamped to
+   * the nearest representable value.
    */
   time::milliseconds
-  getInterestLifetime() const noexcept
-  {
-    return m_interestLifetime;
-  }
+  getInterestLifetime() const noexcept;
 
   /**
    * @brief Set the %Interest's lifetime.
@@ -508,7 +517,7 @@ class Interest : public PacketBase, public std::enable_shared_from_this<Interest
   Name m_name;
   std::vector<Name> m_forwardingHint;
   mutable std::optional<Nonce> m_nonce;
-  time::milliseconds m_interestLifetime = DEFAULT_INTEREST_LIFETIME;
+  uint64_t m_interestLifetime = DEFAULT_INTEREST_LIFETIME.count();
   std::optional<uint8_t> m_hopLimit;
   bool m_canBePrefix = false;
   bool m_mustBeFresh = false;

diff --git a/ndn-cxx/meta-info.cpp b/ndn-cxx/meta-info.cpp
@@ -1,6 +1,6 @@
 /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
 /*
- * Copyright (c) 2013-2023 Regents of the University of California.
+ * Copyright (c) 2013-2024 Regents of the University of California.
  *
  * This file is part of ndn-cxx library (NDN C++ library with eXperimental eXtensions).
  *
@@ -43,7 +43,7 @@ MetaInfo::setType(uint32_t type)
 }
 
 time::milliseconds
-MetaInfo::getFreshnessPeriod() const
+MetaInfo::getFreshnessPeriod() const noexcept
 {
   if (m_freshnessPeriod > static_cast<uint64_t>(time::milliseconds::max().count())) {
     return time::milliseconds::max();
@@ -144,7 +144,7 @@ MetaInfo::wireEncode(EncodingImpl<TAG>& encoder) const
   }
 
   // FreshnessPeriod
-  if (m_freshnessPeriod != static_cast<uint64_t>(DEFAULT_FRESHNESS_PERIOD.count())) {
+  if (m_freshnessPeriod != DEFAULT_FRESHNESS_PERIOD.count()) {
     totalLength += prependNonNegativeIntegerBlock(encoder, tlv::FreshnessPeriod, m_freshnessPeriod);
   }
 
@@ -205,7 +205,7 @@ MetaInfo::wireDecode(const Block& wire)
     ++val;
   }
   else {
-    m_freshnessPeriod = static_cast<uint64_t>(DEFAULT_FRESHNESS_PERIOD.count());
+    m_freshnessPeriod = DEFAULT_FRESHNESS_PERIOD.count();
   }
 
   // FinalBlockId

diff --git a/ndn-cxx/meta-info.hpp b/ndn-cxx/meta-info.hpp
@@ -1,6 +1,6 @@
 /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
 /*
- * Copyright (c) 2013-2023 Regents of the University of California.
+ * Copyright (c) 2013-2024 Regents of the University of California.
  *
  * This file is part of ndn-cxx library (NDN C++ library with eXperimental eXtensions).
  *
@@ -86,18 +86,20 @@ class MetaInfo
   wireDecode(const Block& wire);
 
 public: // getter/setter
-  /** @brief Return the value of `ContentType`.
+  /**
+   * @brief Return the value of `ContentType`.
    *
-   *  If the `ContentType` element is not present, returns tlv::ContentType_Blob.
+   * If the `ContentType` element is not present, returns tlv::ContentType_Blob.
    */
   uint32_t
-  getType() const
+  getType() const noexcept
   {
     return m_type;
   }
 
-  /** @brief Set `ContentType`.
-   *  @param type a number defined in tlv::ContentTypeValue
+  /**
+   * @brief Set the `ContentType`.
+   * @param type A number defined in tlv::ContentTypeValue
    */
   MetaInfo&
   setType(uint32_t type);
@@ -110,10 +112,11 @@ class MetaInfo
    * the nearest representable value.
    */
   time::milliseconds
-  getFreshnessPeriod() const;
+  getFreshnessPeriod() const noexcept;
 
-  /** @brief Set `FreshnessPeriod`.
-   *  @throw std::invalid_argument specified FreshnessPeriod is negative
+  /**
+   * @brief Set the `FreshnessPeriod`.
+   * @throw std::invalid_argument specified FreshnessPeriod is negative.
    */
   MetaInfo&
   setFreshnessPeriod(time::milliseconds freshnessPeriod);
@@ -122,13 +125,13 @@ class MetaInfo
    * @brief Return the value of `FinalBlockId`.
    */
   const std::optional<name::Component>&
-  getFinalBlock() const
+  getFinalBlock() const noexcept
   {
     return m_finalBlockId;
   }
 
   /**
-   * @brief Set `FinalBlockId`.
+   * @brief Set the `FinalBlockId`.
    */
   MetaInfo&
   setFinalBlock(std::optional<name::Component> finalBlockId);
@@ -196,7 +199,7 @@ class MetaInfo
 
 private:
   uint32_t m_type = tlv::ContentType_Blob;
-  uint64_t m_freshnessPeriod = static_cast<uint64_t>(DEFAULT_FRESHNESS_PERIOD.count());
+  uint64_t m_freshnessPeriod = DEFAULT_FRESHNESS_PERIOD.count();
   std::optional<name::Component> m_finalBlockId;
   std::list<Block> m_appMetaInfo;