Project

General

Profile

Bug #10334

cleanup smatch errors in usr/src/lib/fm/topo/libtopo

Added by Rob Johnston 12 months ago. Updated 12 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
-
Start date:
2019-02-01
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:

Description

Currently smatch checking is disabled for usr/src/lib/fm/topo/libtopo. If you un it on, the following smatch issues are reported:

/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_2xml.c:207 txml_print_prop() error: snprintf() chops off the last chars of '"uint32_array"': 13 vs 10
/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_2xml.c:207 txml_print_prop() error: snprintf() chops off the last chars of '"uint32_array"': 13 vs 10
/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_2xml.c:239 txml_print_pgroup() warn: sizeof(NUMBER)?
/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_2xml.c:265 txml_print_node() warn: sizeof(NUMBER)?
/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_2xml.c:285 txml_print_range() warn: sizeof(NUMBER)?
/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_2xml.c:286 txml_print_range() warn: sizeof(NUMBER)?
/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_2xml.c:239 txml_print_pgroup() warn: sizeof(NUMBER)?
/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_2xml.c:265 txml_print_node() warn: sizeof(NUMBER)?
/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_2xml.c:285 txml_print_range() warn: sizeof(NUMBER)?
/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_2xml.c:286 txml_print_range() warn: sizeof(NUMBER)?
/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_mod.c:477 topo_mod_memfmri() warn: inconsistent indenting
/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_module.c:125 topo_mod_stop() error: unchecked function return 'mod->tm_mops->mop_fini'
/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_module.c:184 topo_mod_destroy() warn: variable dereferenced before check 'mod' (see line 182)
/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_node.c:136 topo_node_destroy() warn: variable dereferenced before check 'node' (see line 132)
/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_subr.c:168 topo_debug_set() warn: if statement not indented

Ths issue is to covery addressing all of the above issues and then modifying the libtopo's makefile to enable match for this directory going forward.

History

#1

Updated by Rob Johnston 12 months ago

/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_2xml.c:207 txml_print_prop() error: snprintf() chops off the last chars of '"uint32_array"': 13 vs 10

There were a bunch of places in txml_print_prop() that passed a hardcoded size (10) to snprintf and in many cases, this was too small to hold the string and is also much smaller than the actual buffer size. I changed all these spots to pass the actual size of the buffer.

/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_2xml.c:239 txml_print_pgroup() warn: sizeof(NUMBER)?
/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_2xml.c:265 txml_print_node() warn: sizeof(NUMBER)?
/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_2xml.c:285 txml_print_range() warn: sizeof(NUMBER)?
/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_2xml.c:286 txml_print_range() warn: sizeof(NUMBER)?
/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_2xml.c:239 txml_print_pgroup() warn: sizeof(NUMBER)?
/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_2xml.c:265 txml_print_node() warn: sizeof(NUMBER)?
/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_2xml.c:285 txml_print_range() warn: sizeof(NUMBER)?
/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_2xml.c:286 txml_print_range() warn: sizeof(NUMBER)?

Are the places above are using snprintf to format a string for dumping an integer value. The code was passing a macro as the size param and that macro used faulty logic to calculate the size of a buffer for holding the string representation of a 32-bit integer. Kinda surprised this never blew up.

/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_mod.c:477 topo_mod_memfmri() warn: inconsistent indenting

Removed the nvlist_free call - it's clearly incorrect and looks like an unintentional addition.

/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_module.c:125 topo_mod_stop() error: unchecked function return 'mod->tm_mops->mop_fini'

The mop_fini entry point returns NULL but in intentionally ignored in this case. So I just void-casted the call.

/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_module.c:184 topo_mod_destroy() warn: variable dereferenced before check 'mod' (see line 182)
/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_node.c:136 topo_node_destroy() warn: variable dereferenced before check 'node' (see line 132)

In both of these cases, I moved the offending dereferences to after the NULL-check

/export/home/rejohnst/ws/illumos-upstream1/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../common/topo_subr.c:168 topo_debug_set() warn: if statement not indented

Just a formating fix. Surprised that cstyle never caught this.

#2

Updated by Rob Johnston 12 months ago

Testing

I onu'd a system with these changes and verified that I could successfully take a topo snapshot. Most of the changes affect the code which handles taking a topo snapshot and serializing it to XML. So I captured "fmtopo -x" output before and after these changes and verified that there were no unexpected differences in the generated XML and that the value for the "type" attribute was no longer being truncated (see diff below)

$ diff /var/tmp/fmtopox-before.out ~/fmtopox.after.out 
4c4
<  This topology map file was generated on Feb 04 02:40:24 for openindiana
---
>  This topology map file was generated on Feb 04 02:45:46 for openindiana
984c984
< <propval name='module' type='fmri' value='mod:///mod-name=e1000g/mod-id=180' />
---
> <propval name='module' type='fmri' value='mod:///mod-name=e1000g/mod-id=183' />
990c990
< <propval name='assigned-addresses' type='uint32_ar' value='0x83020010 0x0 0xfd5c0000 0x0 0x20000 0x83020018 0x0 0xfdff0000 0x0 0x10000 0x81020020 0x0 0x2000 0x0 0x40' />
---
> <propval name='assigned-addresses' type='uint32_array' value='0x83020010 0x0 0xfd5c0000 0x0 0x20000 0x83020018 0x0 0xfdff0000 0x0 0x10000 0x81020020 0x0 0x2000 0x0 0x40' />
1029c1029
< <propval name='module' type='fmri' value='mod:///mod-name=audioens/mod-id=210' />
---
> <propval name='module' type='fmri' value='mod:///mod-name=audioens/mod-id=206' />
1035c1035
< <propval name='assigned-addresses' type='uint32_ar' value='0x81020810 0x0 0x2040 0x0 0x40' />
---
> <propval name='assigned-addresses' type='uint32_array' value='0x81020810 0x0 0x2040 0x0 0x40' />
1074c1074
< <propval name='module' type='fmri' value='mod:///mod-name=ahci/mod-id=196' />
---
> <propval name='module' type='fmri' value='mod:///mod-name=ahci/mod-id=197' />
1080c1080
< <propval name='assigned-addresses' type='uint32_ar' value='0x82021824 0x0 0xfd5ef000 0x0 0x1000' />
---
> <propval name='assigned-addresses' type='uint32_array' value='0x82021824 0x0 0xfd5ef000 0x0 0x1000' />
#3

Updated by Electric Monk 12 months ago

  • Status changed from New to Closed
  • % Done changed from 0 to 100

git commit 1e95bfe1233bd7a59421a96c6fa25601561073f3

commit  1e95bfe1233bd7a59421a96c6fa25601561073f3
Author: Rob Johnston <rob.johnston@joyent.com>
Date:   2019-02-06T00:50:51.000Z

    10334 cleanup smatch errors in usr/src/lib/fm/topo/libtopo
    Reviewed by: Andrew Stormont <andyjstormont@gmail.com>
    Reviewed by: Gergő Mihály Doma <domag02@gmail.com>
    Reviewed by: John Levon <john.levon@joyent.com>
    Approved by: Dan McDonald <danmcd@joyent.com>

Also available in: Atom PDF