Uninitialized variables (Lint Warning 603)
This issue was migrated from bug 2812 in our old bug tracker.
Original bug information:
Reporter: Christopher Maynard
Status: RESOLVED LATER
Product: Wireshark
Component: GTK+ UI
OS: All
Platform: x86
Version: Git
Activity
Build Information:
SVN 26033
I ran lint on *.c, epan/*.c, epan/dissectors/packet-*.c, and plugins/*/packet-*.c and the following files were identified by lint as having uninitialized variables. 1) packet-acn.c(528): Warning 603: Symbol 'addr' (line 468) not initialized 2) packet-dnp.c(1560) : Warning 603: Symbol 'al_cto' (line 1255) not initialized 3) packet-lapd.c(284) : Warning 603: Symbol 'data' (line 215) not initialized 4) randpkt.c(544) : Warning 603: Symbol 'ps_header' (line 444) not initialized I checked them and I believe the warnings to be valid. - Chris (BTW: It is not my intent to waste anyone's time with having to look through tons of lint output, most of it harmless, so rather than submit all lint output for all files, I am attempting to identify the more harmful cases and submit each type as I can go through them and try to verify them. If anyone is interested though, I can provide a tar-zip of lint output for each source file that I have run lint against. I am not always in the best position to identify potential problems whereas the original author or core developers might be. Or you may simply have a particular interest in one or more dissectors and would want to scan the output yourself. In any case, the tar-zip is likely to be a large file, so I'm not exactly sure where the best place to post it would be? If there's no place for it on www.wireshark.org, I can see about making it available for ftp download from my company's ftp server.)added uigtk versiondev labels
-
(In reply to comment #0) > SVN 26033 > I ran lint on *.c, epan/*.c, epan/dissectors/packet-*.c, and > plugins/*/packet-*.c and the following files were identified by lint as having > uninitialized variables. Thanks as always for your help, Chris! > 1) packet-acn.c(528): Warning 603: Symbol 'addr' (line 468) not initialized This has since moved to line 465 as of svn rev 26280. (Thinking out loud to understand it myself...) The variable addr is only set using the SET_ADDRESS() macro in two of the three case statements before being used in a call to proto_item_append_text(). The third case statement (ACN_ADDR_IPPORT) uses it in a call to address_to_str(), which in turn passes the value to address_to_str_buf() before initializing it. This must be what lint is complaining about. Simply initializing it to NULL in the declaration won't help matters because address_to_str_buf() will not be happy when addr->type is used in a switch statement. It looks like our goal here is to keep it from being used before initializing it to something useful. I don't know why address_to_str(&addr) is even being used in the third case because it is case ACN_ADDR_IPPORT and should probably only display the port instead IP & port. Anyone familiar with this dissector to know for sure? If not, I can just make an educated guess and change it to avoid potential crashes if/when that code is reached. > 2) packet-dnp.c(1560) : Warning 603: Symbol 'al_cto' (line 1255) not > initialized This looks like another case of using the variable in a case statement without initializing it first while using it in another case statement that does initialize it first. > 3) packet-lapd.c(284) : Warning 603: Symbol 'data' (line 215) not initialized > 4) randpkt.c(544) : Warning 603: Symbol 'ps_header' (line 444) not initialized Chris, are you coming to the same conclusions that I am for #1 and #2? I haven't looked at #3 or #4 yet, but they're probably quite similar. I better not make any changes to the svn trunk right now as it's the middle of the night and I'm tired :). -
For packet-acn.c, I think you nailed it. I don't know much of this protocol so I didn't know if perhaps the address information was actually available in the case of ACN_ADDR_IPPORT, but the author just forgot to call SET_ADDRESS() before the proto_item_append_text() like the other cases? But if the address is not available, then I think removing the call to address_to_str() and %s formatting is probably the best solution there.
For packet-dnp.c, I don't know if a call to dnp3_al_get_timestamp() is the answer there or not in the case of AL_OBJ_BIC_RTIME, similar to the AL_OBJ_TD + AL_OBJ_TDCTO case. Or quite possibly it's fine as it is since it's all wrapped in a for() loop going back to line 1390 and perhaps you'll never see a case of AL_OBJ_BIC_RTIME without a prior AL_OBJ_TD or AL_OBJ_TDCTO within this protocol? If so, maybe just initializing al_cto to zero is good enough? Unfortunately, I don't know anything about dnp either to make a good enough determination for the best fix here. :(
For packet-lapd.c, (the line #'s are off by 2 now in latest SVN), at first glance, it looks like it could end up calling g_memdup() on some potentially uninitialized data and passing it to tvb_new_real_data(). But after looking again, perhaps this one is OK after all since the state is only ever set to DATA when a new_byte() is added to the data[], so at least one valid byte should be in data[]? Maybe a memset(data, 0, sizeof(data)) or something is all that is needed to quiet Lint ... or maybe just ignore this Lint warning altogether? Or maybe it is a problem? I can't quite tell for certain.
For randpkt.c, ps_header is clearly not initialized ... but maybe that's intentional because we're generating random packets? I guess as long as wtap_dump() doesn't care this it's not initialized, then this one could possibly be ignored as well. Or you could zero out the ps_header, which would appease Lint.
-
(In reply to comment #2) > For packet-dnp.c, I don't know if a call to dnp3_al_get_timestamp() is the > answer there or not in the case of AL_OBJ_BIC_RTIME, similar to the AL_OBJ_TD + > AL_OBJ_TDCTO case. Or quite possibly it's fine as it is since it's all wrapped > in a for() loop going back to line 1390 and perhaps you'll never see a case of > AL_OBJ_BIC_RTIME without a prior AL_OBJ_TD or AL_OBJ_TDCTO within this > protocol? If so, maybe just initializing al_cto to zero is good enough? > Unfortunately, I don't know anything about dnp either to make a good enough > determination for the best fix here. :( > As per the protocol spec, you can't have an object with relative time without a preceding common time object, as the time is relative to that. However it's conceivable that a protocol implementation may get this wrong and not provide a CTO, so we should then detect this with an Expert Info error. I'll look into this. -
(In reply to comment #3)
> However it's conceivable that a protocol implementation may get this wrong and
> not provide a CTO, so we should then detect this with an Expert Info error.
> I'll look into this.Graham, were you planning on adding any expert info here?
Or since we've got Clang, Coverity, etc. generating lots of other similar warnings, etc., maybe we should just close this bug as resolved/later? Seems rather pointless to keep this bug open when there are many other similar bugs being reported by other tools that we don't open bugs for.
-
(In reply to comment #4) > (In reply to comment #3) > > However it's conceivable that a protocol implementation may get this wrong and > > not provide a CTO, so we should then detect this with an Expert Info error. > > I'll look into this. > > Graham, were you planning on adding any expert info here? > > Or since we've got Clang, Coverity, etc. generating lots of other similar > warnings, etc., maybe we should just close this bug as resolved/later? Seems > rather pointless to keep this bug open when there are many other similar bugs > being reported by other tools that we don't open bugs for. It's very low on my priority list right now. I'm happy for the bug to be closed.