Repository: randombit/botan
Pull request #3138 (2022-12-20 16:56:01)

Fix overwriting an attribute in an AttributeContainer
If an already added attribute was again added in an `AttributeContainer`, elements were shifted left via move assignment by the `std::remove_if`.
This could result in wrong references to the values of existing attributes.

codecov-commenter: # [Codecov](https://codecov.io/gh/randombit/botan/pull/3138?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd) Report
Base: **87.99**% // Head: **88.01**% // Increases project coverage by **`+0.02%`** :tada:
> Coverage data is based on head [(`e14095b`)](https://codecov.io/gh/randombit/botan/pull/3138?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd) compared to base [(`6863eb2`)](https://codecov.io/gh/randombit/botan/commit/6863eb2e07bd892cc8cee511c8737a126e4958c5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd).
> Patch coverage: 100.00% of modified lines in pull request are covered.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #3138      +/-   ##
==========================================
+ Coverage   87.99%   88.01%   +0.02%     
==========================================
  Files         599      599              
  Lines       66337    66348      +11     
  Branches     6612     6613       +1     
==========================================
+ Hits        58370    58394      +24     
+ Misses       5182     5168      -14     
- Partials     2785     2786       +1     
```


| [Impacted Files](https://codecov.io/gh/randombit/botan/pull/3138?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd) | Coverage Δ | |
|---|---|---|
| [src/lib/prov/pkcs11/p11\_object.cpp](https://codecov.io/gh/randombit/botan/pull/3138/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd#diff-c3JjL2xpYi9wcm92L3BrY3MxMS9wMTFfb2JqZWN0LmNwcA==) | `91.91% <100.00%> (+14.46%)` | :arrow_up: |
| [src/tests/test\_pkcs11\_high\_level.cpp](https://codecov.io/gh/randombit/botan/pull/3138/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd#diff-c3JjL3Rlc3RzL3Rlc3RfcGtjczExX2hpZ2hfbGV2ZWwuY3Bw) | `96.53% <100.00%> (+0.05%)` | :arrow_up: |
| [src/lib/utils/thread\_utils/semaphore.cpp](https://codecov.io/gh/randombit/botan/pull/3138/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd#diff-c3JjL2xpYi91dGlscy90aHJlYWRfdXRpbHMvc2VtYXBob3JlLmNwcA==) | `45.45% <0.00%> (-54.55%)` | :arrow_down: |
| [src/lib/misc/cryptobox/cryptobox.cpp](https://codecov.io/gh/randombit/botan/pull/3138/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd#diff-c3JjL2xpYi9taXNjL2NyeXB0b2JveC9jcnlwdG9ib3guY3Bw) | `90.14% <0.00%> (-2.82%)` | :arrow_down: |
| [src/fuzzer/oaep.cpp](https://codecov.io/gh/randombit/botan/pull/3138/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd#diff-c3JjL2Z1enplci9vYWVwLmNwcA==) | `44.44% <0.00%> (-1.99%)` | :arrow_down: |
| [src/lib/pubkey/mce/mceliece\_key.cpp](https://codecov.io/gh/randombit/botan/pull/3138/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd#diff-c3JjL2xpYi9wdWJrZXkvbWNlL21jZWxpZWNlX2tleS5jcHA=) | `75.38% <0.00%> (-1.03%)` | :arrow_down: |
| [src/tests/test\_bigint.cpp](https://codecov.io/gh/randombit/botan/pull/3138/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd#diff-c3JjL3Rlc3RzL3Rlc3RfYmlnaW50LmNwcA==) | `90.40% <0.00%> (-0.03%)` | :arrow_down: |
| [src/lib/pubkey/dl\_group/dl\_group.cpp](https://codecov.io/gh/randombit/botan/pull/3138/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd#diff-c3JjL2xpYi9wdWJrZXkvZGxfZ3JvdXAvZGxfZ3JvdXAuY3Bw) | `85.09% <0.00%> (+0.67%)` | :arrow_up: |
| [src/fuzzer/mode\_padding.cpp](https://codecov.io/gh/randombit/botan/pull/3138/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd#diff-c3JjL2Z1enplci9tb2RlX3BhZGRpbmcuY3Bw) | `94.64% <0.00%> (+11.95%)` | :arrow_up: |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/randombit/botan/pull/3138?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jack+Lloyd).

lieser: > The free-standing std::remove_if() did actually move-construct some elements in the list to push "to-be-removed" items to the back of the list. Despite the underlying container being a linked-list. Right?

Yes

> Now, I assume that C++20's std::list::remove_if takes advantage of the linked-list data structure and just re-wires the elements as one would expect, leaving references to the "shifted" elements intact.

Yes.
But note that at least on https://en.cppreference.com/w/cpp/container/list/remove I do not see it being mentioned as a guarantee. It is however guaranteed for the [`erase`](https://en.cppreference.com/w/cpp/container/list/erase), which hopefully means it will essentially also hold for the `remove`.

Example that shows the difference: https://godbolt.org/z/KecsoM4b1

Patch:
diff --git a/src/lib/prov/pkcs11/p11_object.cpp b/src/lib/prov/pkcs11/p11_object.cpp
index 7e21ee17b..36ce5a240 100644
--- a/src/lib/prov/pkcs11/p11_object.cpp
+++ b/src/lib/prov/pkcs11/p11_object.cpp
@@ -51,21 +51,20 @@ void AttributeContainer::add_attribute(AttributeType attribute, const uint8_t* v
       if(existing_attribute.type == static_cast< CK_ATTRIBUTE_TYPE >(attribute))
          {
          // remove old entries
-         m_strings.erase(std::remove_if(m_strings.begin(), m_strings.end(), [ &existing_attribute ](const std::string& data)
+         m_strings.remove_if([ &existing_attribute ](const std::string& data)
             {
             return data.data() == existing_attribute.pValue;
-            }), m_strings.end());
+            });
 
-         m_numerics.erase(std::remove_if(m_numerics.begin(), m_numerics.end(), [ &existing_attribute ](const uint64_t& data)
+         m_numerics.remove_if([ &existing_attribute ](const uint64_t& data)
             {
             return &data == existing_attribute.pValue;
-            }), m_numerics.end());
+            });
 
-         m_vectors.erase(std::remove_if(m_vectors.begin(),
-                                        m_vectors.end(), [ &existing_attribute ](const secure_vector<uint8_t>& data)
+         m_vectors.remove_if([ &existing_attribute ](const secure_vector<uint8_t>& data)
             {
             return data.data() == existing_attribute.pValue;
-            }), m_vectors.end());
+            });
 
          existing_attribute.pValue = const_cast<uint8_t*>(value);
          existing_attribute.ulValueLen = size;


Test patch:
diff --git a/src/tests/test_pkcs11_high_level.cpp b/src/tests/test_pkcs11_high_level.cpp
index 5583819aa..7b8d2d438 100644
--- a/src/tests/test_pkcs11_high_level.cpp
+++ b/src/tests/test_pkcs11_high_level.cpp
@@ -489,9 +489,24 @@ Test::Result test_attribute_container()
    attributes.add_binary(AttributeType::Value, bin);
 
    attributes.add_bool(AttributeType::Sensitive, true);
-   attributes.add_numeric(AttributeType::ObjectId, 12);
-
-   result.test_eq("Five elements in attribute container", attributes.count(), 5);
+   attributes.add_numeric(AttributeType::ObjectId, 10);
+   attributes.add_numeric(AttributeType::Id, 20);
+   attributes.add_numeric(AttributeType::PixelX, 30);
+   // Test that overwriting the existing Id attribute works. The numeric attributes above should not be affected by this.
+   attributes.add_numeric(AttributeType::Id, 21);
+   attributes.add_numeric(AttributeType::PixelY, 40);
+
+   result.test_eq("8 elements in attribute container", attributes.count(), 8);
+
+   const std::vector<Botan::PKCS11::Attribute>& storedAttributes = attributes.attributes();
+   result.test_int_eq("ObjectId type", storedAttributes.at(4).type, AttributeType::ObjectId);
+   result.test_int_eq("ObjectId value", *reinterpret_cast< uint64_t* >(storedAttributes.at(4).pValue), 10);
+   result.test_int_eq("Id type", storedAttributes.at(5).type, AttributeType::Id);
+   result.test_int_eq("Id value", *reinterpret_cast< uint64_t* >(storedAttributes.at(5).pValue), 21);
+   result.test_int_eq("PixelX type", storedAttributes.at(6).type, AttributeType::PixelX);
+   result.test_int_eq("PixelX value", *reinterpret_cast< uint64_t* >(storedAttributes.at(6).pValue), 30);
+   result.test_int_eq("PixelY type", storedAttributes.at(7).type, AttributeType::PixelY);
+   result.test_int_eq("PixelY value", *reinterpret_cast< uint64_t* >(storedAttributes.at(7).pValue), 40);
 
    return result;
    }