Fix uninitialized access to valutable_buffer
AbandonedPublic

Authored by aheinecke on Jul 4 2018, 6:24 PM.

Details

Summary
  • src/estream-printf.c (_gpgrt_estream_format): Initialize

valuetable_buffer.

This fixes a crash when the value does not matches the format
string.

GnuPG-Bug-Id: T4054

Test Plan

Before this change it crashed. Afterwards it does not.
I would have pushed it directly but I'm confused why this then
results in the broken message beeing hidden and not in (null)
to be printed. So I might be missing something important here.

Diff Detail

Repository
rE libgpg-error
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
aheinecke created this revision.Jul 4 2018, 6:24 PM
werner added a comment.Jul 4 2018, 7:59 PM

The patch fixes a symptom of wrong format specs usage. What happens with %s with no supplied arg depends on the platform and what is currently on the stack. So it will always be incorrect and you can't do anything about it except for letting the gettext tools checking the PO files for correct format specifier usage. In the english version gcc does the check.

However, while looking at that case I found a slight discrepant between using less than 8 format specifiers and using more. In the latter case the VALUETABLE is allocate using calloc and thus everything is cleared. In the former case only the .vt member is cleared and the other members are not initialized. That is still okay but to avoid surprises I would suggest to clear them all out. This should be done later in the function; something like

   valuetable[validx].vt = VALTYPE_UNSUPPORTED;
+  memset (valuetable[validx].value, 0, sizeof *(valuetable[validx].value));
aheinecke abandoned this revision.Jul 5 2018, 8:54 AM

I agree that the underlying problem is something else but I also think that if a function can avoid a crash on bad input it should try to do so (or at least assert).

So I've commited: fe2f8fca3114e3a5727fdbbc5e7ebc4e442d0401
With the value initialization you suggested.

Tested that it no longer crashes on Windows with that change.