> On October 14, 2015 at 3:20 PM Edward Bartolo <edbarx@???> wrote:
>
>
> This is another part of the backend code where valgrind is saying:
>
> ==5501== 5 errors in context 1 of 3:
> ==5501== Use of uninitialised value of size 8
> ==5501== at 0x5172AFC: ____strtod_l_internal (strtod_l.c:889)
> ==5501== by 0x403856: getRadiatingWifiList (automated_scanner.c:265)
> ==5501== by 0x403BDC: autoWirelessScanPlus (automated_scanner.c:386)
> ==5501== by 0x40400D: autoWirelessScanPlus_RN (automated_scanner.c:549)
> ==5501== by 0x402E2C: main (backend.c:251)
> ==5501== Uninitialised value was created by a stack allocation
> ==5501== at 0x4034BB: getRadiatingWifiList (automated_scanner.c:155)
>
>
> The code portion is this:
> tmp_wifi_quality = calloc(sizeof(wifi_quality),
> 1);
>
> Here follows testing of return value from calloc, but I am not quoting it.
>
> active_wifi_list[*active_wifis] =
> tmp_wifi_quality;
>
> char* substr = strstr((char *) scan_buffer, "Signal level=");
> substr = strstr(substr, "=");
> char* endstr = strstr(substr + 1, " ");
> char tmpstr[MAX_ESSID_LENGTH];
> strncpy(tmpstr, substr + 1, endstr - substr - 1);
> tmpstr[endstr - substr + 1] = '\0';
>
> tmp_wifi_quality->quality = strtod(tmpstr, NULL);
>
> Needless to state, the above works, but valgrind complains.
This diagnostic bothers me:
> ==5501== Uninitialised value was created by a stack allocation
> ==5501== at 0x4034BB: getRadiatingWifiList (automated_scanner.c:155)
This is hundreds of lines away from
> ==5501== by 0x403856: getRadiatingWifiList (automated_scanner.c:265)
which is presumably
> tmp_wifi_quality->quality = strtod(tmpstr, NULL);
You should probably investigate the area around line 155.
=========================
I have some other other comments.
> tmp_wifi_quality = calloc(sizeof(wifi_quality),
> 1);
The canonical way to write this is
> tmp_wifi_quality = calloc(1,
> sizeof(wifi_quality));
The calloc call is designed to return an array of N structures properly aligned
for the requirements of the machine (for embedded pointers, as an example). You
get away from this because N = 1 and it is indistinguishable from returning one
structure aligned on the machine's best array (of bytes). It would not work
right if you needed 2 structures.
I am unhappy with the way substr and endstr are computed because there is no way
to know locally that these searches will not return NULL. If they do, it is
disastrous. It would be better to test for NULL and bail out.
I know you have said earlier that you apply some global constraints to make some
of this impossible, but I believe that local checks (and asserts) lead to better
code.
> char tmpstr[MAX_ESSID_LENGTH];
You are using MAX_ESSID_LENGTH as a really big value for the length of the
signal strength value. OK, but it obscures the semantics. There is probably a
better way to do this.
I think it's worth your while to define a function that replaces these
> strncpy(tmpstr, substr + 1, endstr - substr - 1);
with these
> safe_strncpy(tmpstr, sizeof tempstr, substr + 1, endstr - substr - 1);
which also can guarantee the terminating nul in the destination.
Peter Olson
::
Re: [DNG] int essid_alloc is causin…
