pn532_uart.c:293:5: warning: Use of memory after it is freed
log_put(LOG_GROUP, LOG_CATEGORY, NFC_LOG_PRIORITY_ERROR, "pn53x_check_communication error on %s", ndd.port);
^ ~~~~~~~~
Calling ioctl flush too fast before actual garbage bytes arrive was useless.
It solves an issue e.g. when config asks for scanning for multiple incompatible serial devices:
One scan can mess up the reader and we've to wait & flush properly for the next driver to be able to scan correctly
Problem reported by Coverity:
CID 1090344 (#1 of 1): Use of untrusted string value (TAINTED_STRING)
. tainted_string: Passing tainted string "res->log_level" to "log_init(nfc_context const *)", which cannot accept tainted data.[show details]
Redundant result check leading to dead code was probably indicative
of a missing return value check of acr122_usb_send_apdu()
Problem reported by Coverity:
at_least: At condition "res < 0", the value of "res" must be at least 12.
cannot_single: At condition "res < 0", the value of "res" cannot be equal to -6.
dead_error_condition: The condition "res < 0" cannot be true.
CID 1090327 (#1 of 1): Logically dead code (DEADCODE)
dead_error_begin: Execution cannot reach this statement "acr122_usb_ack(pnd);".
Problem reported by Coverity:
CID 1090334 (#1 of 1): Big parameter passed by value (PASS_BY_VALUE)
pass_by_value: Passing parameter nt of type nfc_target const (size 291 bytes) by value.
Commit 54729fb458 removed some dead code spotted by Coverity
but it had as effect to trigger a gcc warning, which prefers to see all enum in a switch rather than dead code:
pn53x.c: In function 'pn53x_InJumpForDEP':
pn53x.c:2552:5: warning: enumeration value 'NBR_UNDEFINED' not handled in switch [-Wswitch]
pn53x.c:2552:5: warning: enumeration value 'NBR_847' not handled in switch [-Wswitch]
So both switches were merged, which slightly optimizes the code for speed.
Problem reported by Coverity:
CID 1090321 (#1 of 1): Unchecked return value (CHECKED_RETURN)
unchecked_value: No check of the return value of "pn53x_set_property_bool(pnd, NP_INFINITE_SELECT, true)".
Problem reported by Coverity:
CID 1090322 (#1 of 1): Unchecked return value (CHECKED_RETURN)
unchecked_value: No check of the return value of "pn53x_build_frame(abtFrame, &szFrame, pbtData, szData)".
Note that this could happen e.g. if a fake PN533 sends malicious frames over USB
CID 1090329 (#1 of 1): Overflowed return value (INTEGER_OVERFLOW)
overflow_sink: Overflowed or truncated value (or a value computed from an overflowed or truncated value) "res" used as return value.
Problem reported by Coverity:
at_most: At condition "io_res < 0", the value of "io_res" must be at most -1.
dead_error_condition: The condition "io_res < 0" must be true.
CID 1090328 (#1 of 1): Logically dead code (DEADCODE)
dead_error_line: Execution cannot reach this expression "0" inside statement "return (io_res < 0) ? io_re...".
Problem reported by Coverity:
dead_error_condition: The switch value "nbr" cannot be "NBR_UNDEFINED".
CID 1090326 (#1 of 2): Logically dead code (DEADCODE)
dead_error_begin: Execution cannot reach this statement "case NBR_UNDEFINED:".
This avoids Coverity being unhappy that only lower bound was defined, well I hope
lower_bounds: Checking lower bounds of signed scalar "waiting_time" by "waiting_time > 0".
CID 1090343 (#1 of 1): Untrusted value as argument (TAINTED_SCALAR)
tainted_data: Passing tainted variable "waiting_time" to a tainted sink.
sleep(waiting_time);
Problem reported by Coverity:
CID 1090323 (#1 of 1): Unchecked return value (CHECKED_RETURN)
unchecked_value: No check of the return value of "nfc_initiator_select_passive_target(pnd, nmMifare, nt.nti.nai.abtUid, nt.nti.nai.szUidLen, NULL)".
Problem reported by Coverity:
CID 1090325 (#1 of 1): Unchecked return value (CHECKED_RETURN)
unchecked_value: No check of the return value of "nfc_device_set_property_bool(pnd, NP_AUTO_ISO14443_4, false)".
Problem reported by Coverity:
CID 1090318 (#1 of 1): Truncated stdio return value (CHAR_IO)
char_io: Assigning the return value of "getchar(void)" to char "input" truncates its value
Hopefully fix TOCTOU by calling fopen() before stat()
At least this should prevent Coverity to complain about it:
CID 1090346 (#1 of 1): Time of check time of use (TOCTOU)
fs_check_call: Calling function "stat(char const *, struct stat *)" to perform check on "filename".
toctou: Calling function "fopen(char const * restrict, char const * restrict)" that uses "filename" after a check function. This can cause a time-of-check, time-of-use race condition.
Note that it seems pretty hard to avoid completely:
https://en.wikipedia.org/wiki/Time_of_check_to_time_of_use#Preventing_TOCTTOU
switch case was redundant as getopt was already telling the issue:
nfc-read-forum-tag3: option requires an argument -- 'o'
Option -o requires an argument.
This fixes also a problem reported by Coverity about missing break:
CID 1090330 (#1 of 1): Missing break in switch (MISSING_BREAK)
unterminated_case: This case (value 63) is not terminated by a 'break' statement.
Fix warning
uart.c:146:3: warning: ignoring return value of 'read', declared with attribute warn_unused_result [-Wunused-result]
Also reported by Coverity:
CID undefined (#1 of 1): Ignoring number of bytes read (CHECKED_RETURN)
check_return: "read(int, void *, size_t)" returns the number of bytes read, but it is ignored.
Problem reported by Coverity:
CID 1090340 (#1 of 2): Copy into fixed size buffer (STRING_OVERFLOW)
fixed_size_dest: You might overrun the 256 byte fixed-size string "context->user_defined_devices[context->user_defined_device_count - 1U].name" by copying "value" without checking the length.
parameter_as_source: Note: This defect has an elevated risk because the source argument is a parameter of the current function.
CID 1090340 (#2 of 2): Copy into fixed size buffer (STRING_OVERFLOW)[select issue]
Problem reported by Coverity:
CID 1091328 (#1 of 1): Out-of-bounds access (OVERRUN)
overrun-buffer-arg: Overrunning buffer pointed to by "&abtTxBuf[6]" of 271 bytes by passing it to a function which accesses it at byte offset 271 using argument "szData" (which evaluates to 266).
Problem reported by Coverity
CID 1090319 (#1 of 1): Unchecked return value (CHECKED_RETURN)
unchecked_value: No check of the return value of "nfc_device_set_property_bool(pnd, NP_EASY_FRAMING, nt.nti.nai.btSak & 0x20)".
CID 1090320 (#1 of 1): Unchecked return value (CHECKED_RETURN)
unchecked_value: No check of the return value of "nfc_device_set_property_bool(dev, NP_HANDLE_CRC, false)".
CID 1090324 (#1 of 2): Unchecked return value (CHECKED_RETURN)
unchecked_value: No check of the return value of "nfc_device_set_property_bool(pnd, NP_ACTIVATE_FIELD, true)".
CID 1090325 (#1 of 1): Unchecked return value (CHECKED_RETURN)
unchecked_value: No check of the return value of "nfc_device_set_property_bool(pnd, NP_AUTO_ISO14443_4, false)".
CID 1090331 (#1 of 1): Out-of-bounds access (OVERRUN)
11. overrun-buffer-arg: Overrunning array "pnti->nai.abtUid" of 10 bytes by passing it to a function which accesses it at byte offset 11 using argument "pnti->nai.szUidLen" (which evaluates to 12).
as Coverity fails seeing that szTargetTypes will always be = 0 in the case believed to lead to reading unitialized data in apttTargetTypes.
CID 1090347 (#1 of 1): Uninitialized scalar variable (UNINIT)
4. uninit_use_in_call: Using uninitialized element of array "apttTargetTypes" when calling "pn53x_InAutoPoll(struct nfc_device *, pn53x_target_type const *, size_t const, uint8_t const, uint8_t const, nfc_target *, int const)".
source could be larger than destination
Problem reported by Coverity
CID 1090342 (#1 of 1): Unbounded source buffer (STRING_SIZE)
10. string_size: Passing string "envvar" of unknown size to "strcpy(char * restrict, char const * restrict)", which expects a string of a particular size.
Problems reported by Coverity:
CID 1090335 (#1 of 1): Resource leak (RESOURCE_LEAK)
24. leaked_storage: Variable "acPorts" going out of scope leaks the storage it points to.
CID 1090336 (#1 of 1): Resource leak (RESOURCE_LEAK)
10. leaked_storage: Variable "acPorts" going out of scope leaks the storage it points to.
CID 1090337 (#1 of 1): Resource leak (RESOURCE_LEAK)
21. leaked_storage: Variable "i2cPorts" going out of scope leaks the storage it points to.
CID 1090338 (#1 of 1): Resource leak (RESOURCE_LEAK)
21. leaked_storage: Variable "acPorts" going out of scope leaks the storage it points to.
CID 1090339 (#1 of 1): Resource leak (RESOURCE_LEAK)
23. leaked_storage: Variable "acPorts" going out of scope leaks the storage it points to.
Actually the second part of the condition guaranteed that an out-of-bound read would never occur but now code is neater.
It was: for (j = 0; (j < "too_large_bound") && (const_ca[i].saklist[j] >= 0); j++)
Problem reported by Coverity
CID 1090332 (#1 of 1): Out-of-bounds read (OVERRUN)
67. overrun-local: Overrunning array "const_ca[i].saklist" of 8 4-byte elements at element index 31 (byte offset 124) using index "j" (which evaluates to 31).
A buffer overflow could occur is a triple-size UID card was read with a PN531.
Moreover the way cascade tags were removed was just wrong.
Problem reported by Coverity
CID 1090331 (#1 of 1): Out-of-bounds access (OVERRUN)
10. overrun-buffer-arg: Overrunning buffer pointed to by "&pnti->nai.abtUid[5]" of 10 bytes by passing it to a function which accesses it at byte offset 11 using argument "7UL".
Coverity reported a read out of bounds but actually the real problem if PN531 and triple-size UID will already occur at
memcpy(pnti->nai.abtUid, pbtRawData, pnti->nai.szUidLen); where abtUid is of size 10 and szUidLen of size 12
acr122_usb.c:237:3: warning: Function call argument is an uninitialized value
LOG_HEX(NFC_LOG_GROUP_COM, "TX", abtTx, szTx);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../libnfc/log.h:107:7: note: expanded from macro 'LOG_HEX'
snprintf (__acBuf + __szBuf, sizeof(__acBuf) - __szBuf, "%02x ",((uint8_t *)(pbtData))[__szPos]); \
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Maybe a side effect of the #pragma pack not properly understood by the static analyser
but anyway acr122_usb_frame_template is also used the same way to initialize the other frames
acr122s.c:196:23: warning: The left operand of '<<' is a garbage value
size_t frame_size = FRAME_SIZE(frame);
^ ~~~~~~
acr122s.c:76:24: note: expanded from macro 'FRAME_SIZE'
^
acr122s.c:74:47: note: expanded from macro 'APDU_SIZE'
acr122s.c:331:3: warning: Null pointer passed as an argument to a 'nonnull' parameter
memcpy(buf, data, data_size);
^ ~~~~
pn53x.c:3138:3: warning: Null pointer passed as an argument to a 'nonnull' parameter
memcpy(CHIP_DATA(pnd)->current_target, pnt, sizeof(nfc_target));
^ ~~~
pn53x.c:85:57: warning: Result of 'malloc' is converted to a pointer of type 'nfc_modulation_type', which is incompatible with sizeof operand type 'nfc_modulation'
CHIP_DATA(pnd)->supported_modulation_as_initiator = malloc(sizeof(nfc_modulation) * 9);
^~~~~~ ~~~~~~~~~~~~~~~~~~~~~~
ACR122U:
Through PCSC we couldn't send APDUs to the reader if there was no card
but now that we communicate directly with the reader, we can send APDUs
even if there is no card, so we don't need the Escape mechanism anymore
Philippe Teuwen