search

github / codeql

CodeQL: the libraries and queries that power security researchers around the world, as well as code scanning in GitHub Advanced Security
https://codeql.github.com
MIT License
7.52k stars 1.5k forks source link

LGTM.com - false positive: Too few arguments to formatting function #1981

Closed jessicah closed 4 years ago

jessicah commented 5 years ago

Description of the false positive It appears there is an issue with figuring out the position of the format string with Haiku's kernel-only dprintf function.

URL to the alert on the project page on LGTM.com https://lgtm.com/projects/g/haiku/haiku/snapshot/d26be28c9c1882a2fe59ca80fbcf9410053b590f/files/src/add-ons/kernel/bus_managers/acpi/ACPICAHaiku.cpp?sort=name&dir=ASC&mode=heatmap#xf27d4d8483491c15:1

https://lgtm.com/projects/g/haiku/haiku/snapshot/d26be28c9c1882a2fe59ca80fbcf9410053b590f/files/headers/private/file_systems/QueryParser.h?sort=name&dir=ASC&mode=heatmap#x37242992c01d5a0a:1

I've been experimenting with the query, and with the following changes, I'm seeing two results per dprintf call, one with the format string at index 0 (correct), one with the format string at index 1 (incorrect, seems a glibc GNU extension). Is it possible that it's picking up two conflicting definitions of dprintf? Shouldn't the shadowed definition be excluded?


from FormatLiteral fl, FormattingFunctionCall ffc, int expected, int given, int literal, int total
where ffc = fl.getUse()
  and expected = fl.getNumArgNeeded()
  and given = ffc.getNumFormatArgument()
  and literal = ffc.getFormatParameterIndex()
  and total = ffc.getNumberOfArguments()
  and expected > given
  and total >= 3
  and fl.specsAreKnown()
select ffc,
    "Format expects "+expected.toString()+" arguments but given "+given.toString()+" of "+total.toString()+"\n" +
    "target = " + ffc.getTarget().toString()+" format string at " + literal.toString() +"\n" +
    "first argument = " + ffc.getArgument(1).toString()
jbj commented 5 years ago

Thanks for digging into this. It sounds quite likely that our query is picking up GNU dprintf too and is having trouble distinguishing between the two.

We're already tracking this bug under #1971.

jessicah commented 5 years ago

Ah, I didn't realise he'd filed a ticket, his thinking behind the __func__ macro was a good guess, but invalid :)

geoffw0 commented 4 years ago

This issue should be fixed by https://github.com/Semmle/ql/pull/1987 (please allow approximately 2 weeks for the fix to reach LGTM).