Re: Another wackamole embedded perl patch

Glenn Nielsen wrote:

>Hi Theo,
>
>Attached is a patch to fix the instantiation of the perl hash variables
>which are passed to the dynamic perl functions. The hash key length
>used as the third arg to hv_store needs to be the actual string length
>without the terminating '\0', not the size required for storing in
>memory with the terminating '\0'. Without the patch the hash keys
>"ifname", "ip", "broadcast", and "netmask" were not keys in the
>hash passed to the perl function.
>
>Regards,
>
>Glenn
>
>----------------------------------------------------------------------
>Glenn Nielsen             [email protected] | /* Spelin donut madder    |
>MOREnet System Programming               |  * if iz ina coment.      |
>Missouri Research and Education Network  |  */                       |
>----------------------------------------------------------------------
>  
>
>------------------------------------------------------------------------
>
>--- perl.c.orig Thu Apr 22 14:44:47 2004
>+++ perl.c      Thu Nov 17 09:58:28 2005
>@@ -102,26 +102,26 @@
>     SAVETMPS;                             /* ...is a temporary variable.   */
>     PUSHMARK(SP);                         /* remember the stack pointer    */
>     phv = newHV();
>-    hv_store(phv, "ifname", sizeof("ifname"), sv_2mortal(newSVpv(p->ifname,0)),0);
>-    hv_store(phv, "ip", sizeof("ip"), sv_2mortal(newSVpv(inet_ntoa(p->ipaddr),0)),0);
>-    hv_store(phv, "broadcast", sizeof("broadcast"), sv_2mortal(newSVpv(inet_ntoa(p->bcast),0)),0);
>-    hv_store(phv, "netmask", sizeof("netmask"), sv_2mortal(newSVpv(inet_ntoa(p->netmask),0)),0);
>+    hv_store(phv, "ifname", strlen("ifname"), sv_2mortal(newSVpv(p->ifname,0)),0);
>+    hv_store(phv, "ip", strlen("ip"), sv_2mortal(newSVpv(inet_ntoa(p->ipaddr),0)),0);
>+    hv_store(phv, "broadcast", strlen("broadcast"), sv_2mortal(newSVpv(inet_ntoa(p->bcast),0)),0);
>+    hv_store(phv, "netmask", strlen("netmask"), sv_2mortal(newSVpv(inet_ntoa(p->netmask),0)),0);
>
>     eav = newAV();
>     for(i = 0; i < MAX_DEP_IF && e[i].ipaddr.s_addr != 0; i++) {
>         HV* tmphv;
>         tmphv = newHV();
>-        hv_store(tmphv, "ifname", sizeof("ifname"), sv_2mortal(newSVpv(e[i].ifname,0)),0);
>-        hv_store(tmphv, "ip", sizeof("ip"), sv_2mortal(newSVpv(inet_ntoa(e[i].ipaddr),0)),0);
>-        hv_store(tmphv, "broadcast", sizeof("broadcast"), sv_2mortal(newSVpv(inet_ntoa(e[i].bcast),0)),0);
>-        hv_store(tmphv, "netmask", sizeof("netmask"), sv_2mortal(newSVpv(inet_ntoa(e[i].netmask),0)),0);
>+        hv_store(tmphv, "ifname", strlen("ifname"), sv_2mortal(newSVpv(e[i].ifname,0)),0);
>+        hv_store(tmphv, "ip", strlen("ip"), sv_2mortal(newSVpv(inet_ntoa(e[i].ipaddr),0)),0);
>+        hv_store(tmphv, "broadcast", strlen("broadcast"), sv_2mortal(newSVpv(inet_ntoa(e[i].bcast),0)),0);
>+        hv_store(tmphv, "netmask", strlen("netmask"), sv_2mortal(newSVpv(inet_ntoa(e[i].netmask),0)),0);
>         av_push(eav, (SV*)sv_2mortal(newRV_inc((SV *)tmphv)));
>     }
>     rhv = newHV();
>-    hv_store(rhv, "ifname", sizeof("ifname"), sv_2mortal(newSVpv(r->ifname,0)),0);
>-    hv_store(rhv, "ip", sizeof("ip"), sv_2mortal(newSVpv(inet_ntoa(r->ipaddr),0)),0);
>-    hv_store(rhv, "broadcast", sizeof("broadcast"), sv_2mortal(newSVpv(inet_ntoa(r->bcast),0)),0);
>-    hv_store(rhv, "netmask", sizeof("netmask"), sv_2mortal(newSVpv(inet_ntoa(r->netmask),0)),0);
>+    hv_store(rhv, "ifname", strlen("ifname"), sv_2mortal(newSVpv(r->ifname,0)),0);
>+    hv_store(rhv, "ip", strlen("ip"), sv_2mortal(newSVpv(inet_ntoa(r->ipaddr),0)),0);
>+    hv_store(rhv, "broadcast", strlen("broadcast"), sv_2mortal(newSVpv(inet_ntoa(r->bcast),0)),0);
>+    hv_store(rhv, "netmask", strlen("netmask"), sv_2mortal(newSVpv(inet_ntoa(r->netmask),0)),0);
>
>     XPUSHs((SV*)sv_2mortal(newRV_inc((SV *)phv)));/* push the psuedo interface onto the stack  */
>     XPUSHs((SV*)sv_2mortal(newRV_inc((SV *)eav)));/* push the extra interface array ref onto the stack  */
>  
>


Made a slightly different change.  No since in calling strlen on 
something when your compiler can make it a static numner -- as long as I 
don't give it the wrong static number ;-)

; cvs diff -u
cvs diff: Diffing .
Index: ChangeLog
===================================================================
RCS file: /storage/cvs/munjal/wackamole/ChangeLog,v
retrieving revision 1.27
diff -u -r1.27 ChangeLog
--- ChangeLog   18 Nov 2005 16:49:05 -0000      1.27
+++ ChangeLog   18 Nov 2005 16:53:22 -0000
@@ -89,3 +89,5 @@
  * patch from < pete at more dot net > and < anderson at more dot net >
        regarding uninitialized stack variable usable.  Fixes occasional
        interface operations on FreeBSD.
+ * wrong string length of static strings, off-by-one. from Glenn Nielsen
+       < glenn at maildot more dot net >
Index: perl.c
===================================================================
RCS file: /storage/cvs/munjal/wackamole/perl.c,v
retrieving revision 1.4
diff -u -r1.4 perl.c
--- perl.c      7 May 2005 20:23:27 -0000       1.4
+++ perl.c      18 Nov 2005 16:53:22 -0000
@@ -84,26 +84,26 @@
     SAVETMPS;                             /* ...is a temporary 
variable.   */
     PUSHMARK(SP);                         /* remember the stack 
pointer    */
     phv = newHV();
-    hv_store(phv, "ifname", sizeof("ifname"), 
sv_2mortal(newSVpv(p->ifname,0)),0);
-    hv_store(phv, "ip", sizeof("ip"), 
sv_2mortal(newSVpv(inet_ntoa(p->ipaddr),0)),0);
-    hv_store(phv, "broadcast", sizeof("broadcast"), 
sv_2mortal(newSVpv(inet_ntoa(p->bcast),0)),0);
-    hv_store(phv, "netmask", sizeof("netmask"), 
sv_2mortal(newSVpv(inet_ntoa(p->netmask),0)),0);
+    hv_store(phv, "ifname", sizeof("ifname")-1, 
sv_2mortal(newSVpv(p->ifname,0)),0);
+    hv_store(phv, "ip", sizeof("ip")-1, 
sv_2mortal(newSVpv(inet_ntoa(p->ipaddr),0)),0);
+    hv_store(phv, "broadcast", sizeof("broadcast")-1, 
sv_2mortal(newSVpv(inet_ntoa(p->bcast),0)),0);
+    hv_store(phv, "netmask", sizeof("netmask")-1, 
sv_2mortal(newSVpv(inet_ntoa(p->netmask),0)),0);
 
     eav = newAV();
     for(i = 0; i < MAX_DEP_IF && e[i].ipaddr.s_addr != 0; i++) {
         HV* tmphv;
         tmphv = newHV();
-        hv_store(tmphv, "ifname", sizeof("ifname"), 
sv_2mortal(newSVpv(e[i].ifname,0)),0);
-        hv_store(tmphv, "ip", sizeof("ip"), 
sv_2mortal(newSVpv(inet_ntoa(e[i].ipaddr),0)),0);
-        hv_store(tmphv, "broadcast", sizeof("broadcast"), 
sv_2mortal(newSVpv(inet_ntoa(e[i].bcast),0)),0);
-        hv_store(tmphv, "netmask", sizeof("netmask"), 
sv_2mortal(newSVpv(inet_ntoa(e[i].netmask),0)),0);
+        hv_store(tmphv, "ifname", sizeof("ifname")-1, 
sv_2mortal(newSVpv(e[i].ifname,0)),0);
+        hv_store(tmphv, "ip", sizeof("ip")-1, 
sv_2mortal(newSVpv(inet_ntoa(e[i].ipaddr),0)),0);
+        hv_store(tmphv, "broadcast", sizeof("broadcast")-1, 
sv_2mortal(newSVpv(inet_ntoa(e[i].bcast),0)),0);
+        hv_store(tmphv, "netmask", sizeof("netmask")-1, 
sv_2mortal(newSVpv(inet_ntoa(e[i].netmask),0)),0);
         av_push(eav, (SV*)sv_2mortal(newRV_inc((SV *)tmphv)));
     }   
     rhv = newHV();
-    hv_store(rhv, "ifname", sizeof("ifname"), 
sv_2mortal(newSVpv(r->ifname,0)),0);
-    hv_store(rhv, "ip", sizeof("ip"), 
sv_2mortal(newSVpv(inet_ntoa(r->ipaddr),0)),0);
-    hv_store(rhv, "broadcast", sizeof("broadcast"), 
sv_2mortal(newSVpv(inet_ntoa(r->bcast),0)),0);
-    hv_store(rhv, "netmask", sizeof("netmask"), 
sv_2mortal(newSVpv(inet_ntoa(r->netmask),0)),0);
+    hv_store(rhv, "ifname", sizeof("ifname")-1, 
sv_2mortal(newSVpv(r->ifname,0)),0);
+    hv_store(rhv, "ip", sizeof("ip")-1, 
sv_2mortal(newSVpv(inet_ntoa(r->ipaddr),0)),0);
+    hv_store(rhv, "broadcast", sizeof("broadcast")-1, 
sv_2mortal(newSVpv(inet_ntoa(r->bcast),0)),0);
+    hv_store(rhv, "netmask", sizeof("netmask")-1, 
sv_2mortal(newSVpv(inet_ntoa(r->netmask),0)),0);
    
     XPUSHs((SV*)sv_2mortal(newRV_inc((SV *)phv)));/* push the psuedo 
interface onto the stack  */
     XPUSHs((SV*)sv_2mortal(newRV_inc((SV *)eav)));/* push the extra 
interface array ref onto the stack  */


; cvs commit -m "fix for off-by-one in the C->perl layer.  perl keys in 
the has had the trailing null terminator on them!"
Checking in ChangeLog;
/storage/cvs/munjal/wackamole/ChangeLog,v  <--  ChangeLog
new revision: 1.28; previous revision: 1.27
done
Checking in perl.c;
/storage/cvs/munjal/wackamole/perl.c,v  <--  perl.c
new revision: 1.5; previous revision: 1.4
done
;


-- 
// Theo Schlossnagle
// Principal Engineer -- http://www.omniti.com/~jesus/
// Ecelerity: Run with it. -- http://www.omniti.com/