1

I'm trying to remediate some Coverity findings on tainted values due to the use of atoi and atof. I switched to an istringstream, but its not producing expected results for bases other than 10.

If I switch to base 16, enter 0xa and avoid the iss.ignore(2);, then the result is 0:

$ ./tt.exe 0xa
X: 0

If I switch to base 16, enter 0xa and utilize the iss.ignore(2);, then the result is an exception:

$ ./tt.exe 0xa
'0xa' is not a value

I visited CPP Reference on istringstream as recommended by @πάντα, but it does not discuss a limitation in this context.

Any ideas what I am doing wrong? Or, how can I get this to work as expected?


$ cat tt.cxx
#include <iostream>
#include <sstream>
#include <iomanip>
#include <stdexcept>
using namespace std;

template <class T>
T StringToValue(const std::string& str) {
  std::istringstream iss(str);
  T value;

  if (str.length() >= 2) {
    if (str[0] == '0' && (str[1] =='X' || str[1] =='x'))
      {
        iss.setf(std::ios_base::hex);
        iss.ignore(2);
      }
  }
  iss >> value;

  if (iss.fail())
    throw runtime_error("'" + str +"' is not a value");

  return value;
}

int main(int argc, char* argv[])
{
  try
    {
      int x = StringToValue<int>(argc >= 2 ? argv[1] : "ZZZ...");
      cout << "X: " << x << endl;
    }
  catch(const runtime_error& ex)
    {
      cerr << ex.what() << endl;
      return 1;
    }

  return 0;
}
jww
  • 97,681
  • 90
  • 411
  • 885
  • Get rid of the `0x` prefix when reading in the digits. – πάντα ῥεῖ Nov 14 '15 at 12:23
  • Ah I see, you've placed the `ignore(2)` there. – πάντα ῥεῖ Nov 14 '15 at 12:24
  • The usual way is something like `iss >> hex >> value;`. – πάντα ῥεῖ Nov 14 '15 at 12:26
  • Can you make a _minimal_ testcase please? – Lightness Races in Orbit Nov 14 '15 at 12:32
  • So the problem in that `atoi` and friends not handle errors? Then why not use `std::stoi` and others, they throw exceptions on error. – fghj Nov 14 '15 at 12:35
  • 1
    @user1034749 - we support ancient compilers. In fact, I just finished testing on Fedora 1 with GCC 3.2. (We are not like browsers and Apple operating systems). – jww Nov 14 '15 at 12:38
  • @Lightness Races in Orbit - that is a minimal test case. In fact, I created it just for you :) – jww Nov 14 '15 at 12:40
  • @jww: It's not though, which is why you blamed istringstream when in fact istringstream extracts hexadecimals just fine. The bug is in your code somewhere, and you haven't performed enough debugging to isolate it. – Lightness Races in Orbit Nov 14 '15 at 12:41
  • @jww If it is ancient compiler, than why not `sscanf(str, "%X", &int_val) == 1`? With `gcc -Wformat=error` and `coverity` you will be completely safe, and not have to deal with bugs in old `c++` runtime. – fghj Nov 14 '15 at 12:42
  • @Lightness Races in Orbit - thanks. All the code is above. Feels free to let me know how I screwed up `iss.setf(std::ios_base::hex);` when using `0xa`. That's the reason I provided everything I am doing, and even showed other attempts to get it to work. – jww Nov 14 '15 at 12:43
  • @jww: Off-hand I can't see it (other than the one bug I pointed out in comments below my answer). Perhaps when I get back from where I'm going in a minute I'll have a deeper look. – Lightness Races in Orbit Nov 14 '15 at 12:51
  • @Lightness Races in Orbit - *"...I'll have a deeper look"* - thanks. Its more a morbid curiosity more than anything else. If you don't have time, then that's fine too. Thanks again for the help. – jww Nov 14 '15 at 13:24
  • @user1034749 - its a C++ project, and it has high integrity requirements. As soon as the less-safe functions like `sscanf` are used, then it triggers a security defect. It also causes some folks to fail an audit (if they have C&A requirements). We could probably use `sscanf_s` from ISO/IEC TR 24731-2 on Windows, but then Apple, BSD, Linux and some Unixes would need a separate implementation that does not trigger a security finding, and it passes a C&A audit. – jww Nov 14 '15 at 13:29
  • @LightnessRacesinOrbit - Ah, that makes perfect sense. Thanks again for the help. Sorry about wasting your time with it. – jww Nov 14 '15 at 19:12

1 Answers1

4

You're completely overthinking this. Reading a value in hexadecimal notation is easy.

#include <sstream>
#include <iomanip>
#include <cassert>

int main()
{
   {
      int x = 0;
      std::istringstream is("0xa");
      is >> std::hex >> x;
      assert(x == 10);
   }
   {
      int x = 0;
      std::istringstream is("a");
      is >> std::hex >> x;
      assert(x == 10);
   }
}

(live demo)


Regardless, I can identify two problems with your code.

1. Incorrect use of std::ios_base::setf

You're replacing the stream's entire flagset with only std::ios_base::hex. The flags are not just for numeric bases. To do that, you need to mask out everything else to prevent unsetting other, unrelated, but necessary flags (I don't know what they are):

iss.setf(std::ios_base::hex, std::ios::basefield);

This is why iss >> std::hex is so much easier.

It's also why you should have constructed a minimal testcase consisting of nothing but a test of iss.setf before posting!

2. Broken logic

The case where input is just "a" skips that statement entirely, as it is conditional on the first two characters being "0x".

Move it to just before the iss >> value, I would.


You can see your fixed code here but, as explored at the start of my answer, the whole switching-on-leading-"0x" thing is unnecessary, so most of your code can just be removed.

Lightness Races in Orbit
  • 378,754
  • 76
  • 643
  • 1,055
  • Thanks. Any ideas why explicitly setting the base did not work as expected? It may help me in the future. – jww Nov 14 '15 at 12:36
  • @jww: I didn't look into it. Now that I have proven with a minimal testcase that `istringstream` works just fine, you can continue your debugging to find out. :) – Lightness Races in Orbit Nov 14 '15 at 12:37
  • It looks like it might help others, too: [Get precision of cout](http://stackoverflow.com/q/20374137). Thanks again. – jww Nov 19 '15 at 11:01