- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 25
Replace calls to sprintf with snprintf. #96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
flag this as a deprecated call for security reasons. I.e. reapack did not build for me in a straight up MacOS setup and this PR fixes that. There are no bugs fixed here though it does get rid of the distasteful practice of writing to the nul at the end of a std:string buffer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Looks like the overbearing deprecation comes from XCode 14 specifically (sprintf isn't actually deprecated in the standard). Will accept this PR after the suggested changes.
        
          
                src/hash.cpp
              
                Outdated
          
        
      | char buf[3]; | ||
| snprintf(buf, sizeof(buf), "%02x", multihash[i]); | ||
| m_value[i * 2] = buf[0]; | ||
| m_value[i * 2 + 1] = buf[1]; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no point in writing to a temporary buffer then copying. The direct sprintf to snprintf conversion would be:
| char buf[3]; | |
| snprintf(buf, sizeof(buf), "%02x", multihash[i]); | |
| m_value[i * 2] = buf[0]; | |
| m_value[i * 2 + 1] = buf[1]; | |
| snprintf(&m_value[i * 2], 3, "%02x", multihash[i]); | 
...though there's also no point in writing a temporary null terminator every 2 digits only to overwrite it with the next digit in the next iteration (overwriting std::string's own isn't UB since C++17), so this loop would be better written as:
diff --git c/src/hash.cpp i/src/hash.cpp
index dbe1fc3..e47a01d 100644
--- c/src/hash.cpp
+++ i/src/hash.cpp
@@ -297,8 +297,12 @@ const std::string &Hash::digest()
 
   m_value.resize(multihash.size() * 2);
 
-  for(size_t i = 0; i < multihash.size(); ++i)
-    sprintf(&m_value[i * 2], "%02x", multihash[i]);
+  static const char hex[] = "0123456789abcdef";
+
+  for(size_t i = 0; i < multihash.size(); ++i) {
+    m_value[i * 2    ] = hex[multihash[i] >> 4];
+    m_value[i * 2 + 1] = hex[multihash[i] & 0xf];
+  }
 
   return m_value;
 }There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My first thought was to rewrite it to not use string formatting at all but I figured that would be a larger change. I'll roll your proposed code into the PR in a little bit.
Co-authored-by: Christian Fillion <[email protected]>
sprintf is now deprecated for security reasons. I.e. reapack did not build for me in a straight up MacOS setup and this PR fixes that.
There are no bugs fixed here though it does get rid of the distasteful practice of writing to the nul at the end of a
std::stringbuffer.