Skip to content

Replace vsprintf with vsnprintf#291

Merged
ZedThree merged 3 commits intomasterfrom
vsprintf-fix
Oct 10, 2016
Merged

Replace vsprintf with vsnprintf#291
ZedThree merged 3 commits intomasterfrom
vsprintf-fix

Conversation

@bendudson
Copy link
Contributor

Safe version, which guards against buffer overruns
by specifying the length of the buffer into which text
is being printed.

This was used in both Output and BoutException classes
to provide C printf-style output.

This fixes issue #286

Safe version, which guards against buffer overruns
by specifying the length of the buffer into which text
is being printed.

This was used in both Output and BoutException classes
to provide C printf-style output.
Missed a couple of instances in BoutException
derived classes.
char buffer[1024];
va_start(ap, s);
vsprintf(buffer, s, ap);
vsnprintf(buffer, 1024, s, ap);
Copy link
Member

@ZedThree ZedThree Oct 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be a constant too? BoutException::BUFFERLEN?

Both Output and BoutException now use a static const int
BUFFER_LEN to control the length of the buffer used for
C style output.
@ZedThree ZedThree merged commit 8057d75 into master Oct 10, 2016
@ZedThree ZedThree deleted the vsprintf-fix branch October 10, 2016 16:16
@dschwoerer
Copy link
Contributor

The return value of vsnprintf should be checked, and if smaller than the buffer size, a sufficiently large buffer should be allocated - or an exception should be thrown (easier)

@bendudson
Copy link
Contributor Author

Throwing an exception inside BoutException could end badly. In output I don't think throwing an exception is the best way. These outputs will be mainly used for printing information on a run as it goes, so terminating just because an output couldn't be printed is probably not what the user would want. Possibly allocating more memory, but 1024 characters should be enough for anyone...

One question is how fixes like this should be applied to next. Should there be a second branch from next, which would presumably then conflict in any merge, or wait and merge master into next later?

@dschwoerer
Copy link
Contributor

In output, this would only happen if the path of the file, you are trying to open would be longer than whatever.
In that case, ignoring is terrible, as it might overwrite anything. Maybe something important ...

In exception it shouldn't be to much work to reallocate memory ...

@ZedThree
Copy link
Member

With a bit of luck, next will be ready to be merged into master soon-ish. Then, hopefully the development branch and master won't be too far apart, so hotfixes like this can be applied to both at the same time, though that would have to be done from the command line (or with a second PR, I guess?)

@ZedThree
Copy link
Member

Good point about output filenames.

Linux has a maximum path length of 4096 characters. Although I'm not sure we want to be allocating that much all the time.

@bendudson
Copy link
Contributor Author

ok, this is used in three places:

  1. In Output as a printf() convenience. This doesn't open any files, and only writes text to stdout and the BOUT.log files. Here allocating memory would be nice, but not essential. Throwing an exception is not desirable.

  2. In BoutException to format error messages. I think allocating or reallocating memory here is not advisable, because the exception may have been thrown due to memory corruption and so allocs/frees may cause further exceptions. I think the only option here is to ignore anything not printed.

  3. In DataFile, where it is used to create file names. This still uses vsprintf, not vsnprintf. I agree there is a potential issue with this, and either it needs to allocate more memory or throw an exception.

The correct fix in "next" would be to use variadic templates for the functions, rather than C vararg.

@dschwoerer
Copy link
Contributor

About 1):

  int open(const char *fname, ...); ///< Open an output log file                                                                             

seems to me like it would be opening a file, not writing to it.
On the other hand, I am not sure this is often used ...

about Output::write I think an error here could be ignored ... ( I missed earlier that vs(n)printf is used here as well)

about 2):
We are anyway allocating memory for the buffer, and in general a whole exception is constructed.
I am not sure an extra delete/new call would make a difference.

about 3)
This was the main reason I opened this issue :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants