Mysterious memory corruption bug

Mysterious memory corruption bug

[Bean]

[-- Attachment #1: Type: text/plain, Size: 1475 bytes --]

Hi,

Thanks to Vladimir's memory patch, it's actually quite easy to
reproduce mysterious issue.

First, there are two memory leaks in ip.c.

It allocates the rsm but never frees it. free_rsm frees its content,
but not the pointer itself. You can see it in printmem at ip.c:473
      rsm = grub_malloc (sizeof (*rsm));

Another problem is at ip.c:594:
  return handle_dgram (ret, card, src_hwaddress,
			       hwaddress, proto, &source, &dest,
			       ttl);
here, ret is netbuff. grub_netbuff_alloc get a buffer for both data
and header (data go first), so when it frees the data pointer, the
header goes away as well. But here, the header is allocated separately
so that it's not free using , you can see it from printmem at ip.c:580
      ret = grub_malloc (sizeof (*ret));

Now here's the tricky part, when i fix both problem, it actually when
you call this: (memdisk size is 19,180, just in case it matters).

testspeed /memdisk

So there must be a memory corruption somewhere. (It will not halt if
you skip the the second leak, but you can see the remaining buffer in
printmem).

BTW, you should add a grub_free_fragment call in testspeed to free the
rsm cache, just to make the printmem output a little cleaner.

These are the modules used to generate grub.efi, just in case it's relevant.

/grub-mkimage -d grub-core -o grub.efi -O x86_64-efi chain boot test
fat ntfs part_msdos normal ls echo efinet tftp http efinet reboot
testspeed printmem

-- 
Best wishes
Bean

[-- Attachment #2: test.txt --]
[-- Type: text/plain, Size: 3085 bytes --]

=== modified file 'grub-core/net/ip.c'
--- grub-core/net/ip.c	2012-02-09 22:43:43 +0000
+++ grub-core/net/ip.c	2012-05-01 18:28:59 +0000
@@ -240,7 +240,7 @@
 	FOR_NET_NETWORK_LEVEL_INTERFACES (inf)
 	  if (inf->card == card
 	      && inf->address.type == GRUB_NET_NETWORK_LEVEL_PROTOCOL_DHCP_RECV
-	      && grub_net_hwaddr_cmp (&inf->hwaddress, hwaddress) == 0)
+	      && inf->hwaddress.type == GRUB_NET_LINK_LEVEL_PROTOCOL_ETHERNET)
 	    {
 	      if (udph->chksum)
 		{
@@ -257,18 +257,25 @@
 				    "Expected %x, got %x\n", 
 				    grub_be_to_cpu16 (expected),
 				    grub_be_to_cpu16 (chk));
-		      grub_netbuff_free (nb);
-		      return GRUB_ERR_NONE;
+		      break;
 		    }
 		  udph->chksum = chk;
 		}
 
 	      err = grub_netbuff_pull (nb, sizeof (*udph));
 	      if (err)
-		return err;
-	      grub_net_process_dhcp (nb, inf->card);
-	      grub_netbuff_free (nb);
-	      return GRUB_ERR_NONE;
+		{
+		  grub_netbuff_free (nb);
+		  return err;
+		}
+	      struct grub_net_bootp_packet *dhcp =
+		(struct grub_net_bootp_packet *) nb->data;
+	      if (grub_memcmp(inf->hwaddress.mac, &dhcp->mac_addr,
+			      sizeof(inf->hwaddress.mac)) == 0)
+		{
+		  grub_net_process_dhcp (nb, inf->card);
+		  break;
+		}
 	    }
 	grub_netbuff_free (nb);
 	return GRUB_ERR_NONE;
@@ -344,19 +351,34 @@
     }
   grub_free (rsm->asm_buffer);
   grub_priority_queue_destroy (rsm->pq);
+  grub_free (rsm);
 }
 
 static void
 free_old_fragments (void)
 {
-  struct reassemble *rsm, **prev;
+  struct reassemble *rsm, **prev, *tmp;
   grub_uint64_t limit_time = grub_get_time_ms () - 90000;
 
-  for (prev = &reassembles, rsm = *prev; rsm; prev = &rsm->next, rsm = *prev)
+  for (prev = &reassembles, rsm = *prev; rsm;
+       prev = &rsm->next, rsm = *prev, free_rsm (tmp))
     if (rsm->last_time < limit_time)
       {
 	*prev = rsm->next;
-	free_rsm (rsm);
+	tmp = rsm;	
+      }
+}
+
+void
+grub_free_fragments (void)
+{
+  struct reassemble *rsm, **prev, *tmp;
+
+  for (prev = &reassembles, rsm = *prev; rsm;
+       prev = &rsm->next, rsm = *prev, free_rsm (tmp))
+      {
+	*prev = rsm->next;
+	tmp = rsm;
       }
 }
 
@@ -570,9 +592,14 @@
       dest.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
       dest.ipv4 = dst;
 
-      return handle_dgram (ret, card, src_hwaddress,
-			   hwaddress, proto, &source, &dest,
-			   ttl);
+      {
+	grub_err_t result;	
+	result = handle_dgram (ret, card, src_hwaddress,
+			       hwaddress, proto, &source, &dest,
+			       ttl);
+	grub_free (ret);
+	return result;
+      }
     }
 }
 

=== modified file 'grub-core/net/tftp.c'
--- grub-core/net/tftp.c	2012-02-12 18:11:06 +0000
+++ grub-core/net/tftp.c	2012-05-01 17:22:35 +0000
@@ -320,9 +320,9 @@
   rrqlen += grub_strlen ("blksize") + 1;
   rrq += grub_strlen ("blksize") + 1;
 
-  grub_strcpy (rrq, "1024");
-  rrqlen += grub_strlen ("1024") + 1;
-  rrq += grub_strlen ("1024") + 1;
+  grub_strcpy (rrq, "4096");
+  rrqlen += grub_strlen ("4096") + 1;
+  rrq += grub_strlen ("4096") + 1;
 
   grub_strcpy (rrq, "tsize");
   rrqlen += grub_strlen ("tsize") + 1;

Re: Mysterious memory corruption bug

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1909 bytes --]

On 01.05.2012 20:53, Bean wrote:
> Hi,
>
> Thanks to Vladimir's memory patch, it's actually quite easy to
> reproduce mysterious issue.
>
> First, there are two memory leaks in ip.c.
>
> It allocates the rsm but never frees it. free_rsm frees its content,
> but not the pointer itself. You can see it in printmem at ip.c:473
>       rsm = grub_malloc (sizeof (*rsm));
>
> Another problem is at ip.c:594:
>   return handle_dgram (ret, card, src_hwaddress,
> 			       hwaddress, proto, &source, &dest,
> 			       ttl);
> here, ret is netbuff. grub_netbuff_alloc get a buffer for both data
> and header (data go first), so when it frees the data pointer, the
> header goes away as well. But here, the header is allocated separately
> so that it's not free using , you can see it from printmem at ip.c:580
>       ret = grub_malloc (sizeof (*ret));
>
> Now here's the tricky part, when i fix both problem, it actually when
> you call this: (memdisk size is 19,180, just in case it matters).
>
> testspeed /memdisk
>
> So there must be a memory corruption somewhere.
You can check for memory corruptions by calling grub_mm_check often
enough in the code.
>  (It will not halt if
> you skip the the second leak, but you can see the remaining buffer in
> printmem).
>
> BTW, you should add a grub_free_fragment call in testspeed to free the
> rsm cache, just to make the printmem output a little cleaner.
>
> These are the modules used to generate grub.efi, just in case it's relevant.
>
> /grub-mkimage -d grub-core -o grub.efi -O x86_64-efi chain boot test
> fat ntfs part_msdos normal ls echo efinet tftp http efinet reboot
> testspeed printmem
>
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: Mysterious memory corruption bug

[Bean]

On Wed, May 2, 2012 at 3:08 AM, Vladimir 'φ-coder/phcoder' Serbinenko
<phcoder@gmail.com> wrote:
> On 01.05.2012 20:53, Bean wrote:
>> Hi,
>>
>> Thanks to Vladimir's memory patch, it's actually quite easy to
>> reproduce mysterious issue.
>>
>> First, there are two memory leaks in ip.c.
>>
>> It allocates the rsm but never frees it. free_rsm frees its content,
>> but not the pointer itself. You can see it in printmem at ip.c:473
>>       rsm = grub_malloc (sizeof (*rsm));
>>
>> Another problem is at ip.c:594:
>>   return handle_dgram (ret, card, src_hwaddress,
>>                              hwaddress, proto, &source, &dest,
>>                              ttl);
>> here, ret is netbuff. grub_netbuff_alloc get a buffer for both data
>> and header (data go first), so when it frees the data pointer, the
>> header goes away as well. But here, the header is allocated separately
>> so that it's not free using , you can see it from printmem at ip.c:580
>>       ret = grub_malloc (sizeof (*ret));
>>
>> Now here's the tricky part, when i fix both problem, it actually when
>> you call this: (memdisk size is 19,180, just in case it matters).
>>
>> testspeed /memdisk
>>
>> So there must be a memory corruption somewhere.
> You can check for memory corruptions by calling grub_mm_check often
> enough in the code.

Hi,

Thanks for the tip. But when I add a few grub_mm_check and printf here
and there, it doesn't halt any more. So this could be a memory
overflown issue or even compiler bug, very strange indeed.

-- 
Best wishes
Bean

Re: Mysterious memory corruption bug

[Bean]

On Wed, May 2, 2012 at 3:46 AM, Bean <bean123ch@gmail.com> wrote:
> On Wed, May 2, 2012 at 3:08 AM, Vladimir 'φ-coder/phcoder' Serbinenko
> <phcoder@gmail.com> wrote:
>> On 01.05.2012 20:53, Bean wrote:
>>> Hi,
>>>
>>> Thanks to Vladimir's memory patch, it's actually quite easy to
>>> reproduce mysterious issue.
>>>
>>> First, there are two memory leaks in ip.c.
>>>
>>> It allocates the rsm but never frees it. free_rsm frees its content,
>>> but not the pointer itself. You can see it in printmem at ip.c:473
>>>       rsm = grub_malloc (sizeof (*rsm));
>>>
>>> Another problem is at ip.c:594:
>>>   return handle_dgram (ret, card, src_hwaddress,
>>>                              hwaddress, proto, &source, &dest,
>>>                              ttl);
>>> here, ret is netbuff. grub_netbuff_alloc get a buffer for both data
>>> and header (data go first), so when it frees the data pointer, the
>>> header goes away as well. But here, the header is allocated separately
>>> so that it's not free using , you can see it from printmem at ip.c:580
>>>       ret = grub_malloc (sizeof (*ret));
>>>
>>> Now here's the tricky part, when i fix both problem, it actually when
>>> you call this: (memdisk size is 19,180, just in case it matters).
>>>
>>> testspeed /memdisk
>>>
>>> So there must be a memory corruption somewhere.
>> You can check for memory corruptions by calling grub_mm_check often
>> enough in the code.
>
> Hi,
>
> Thanks for the tip. But when I add a few grub_mm_check and printf here
> and there, it doesn't halt any more. So this could be a memory
> overflown issue or even compiler bug, very strange indeed.

Hi,

Well, actually it does halt with a larger file, so at least the
behavior is predictable.

-- 
Best wishes
Bean

Re: Mysterious memory corruption bug

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 2093 bytes --]

On 01.05.2012 21:52, Bean wrote:
> On Wed, May 2, 2012 at 3:46 AM, Bean <bean123ch@gmail.com> wrote:
>> On Wed, May 2, 2012 at 3:08 AM, Vladimir 'φ-coder/phcoder' Serbinenko
>> <phcoder@gmail.com> wrote:
>>> On 01.05.2012 20:53, Bean wrote:
>>>> Hi,
>>>>
>>>> Thanks to Vladimir's memory patch, it's actually quite easy to
>>>> reproduce mysterious issue.
>>>>
>>>> First, there are two memory leaks in ip.c.
>>>>
>>>> It allocates the rsm but never frees it. free_rsm frees its content,
>>>> but not the pointer itself. You can see it in printmem at ip.c:473
>>>>       rsm = grub_malloc (sizeof (*rsm));
>>>>
>>>> Another problem is at ip.c:594:
>>>>   return handle_dgram (ret, card, src_hwaddress,
>>>>                              hwaddress, proto, &source, &dest,
>>>>                              ttl);
>>>> here, ret is netbuff. grub_netbuff_alloc get a buffer for both data
>>>> and header (data go first), so when it frees the data pointer, the
>>>> header goes away as well. But here, the header is allocated separately
>>>> so that it's not free using , you can see it from printmem at ip.c:580
>>>>       ret = grub_malloc (sizeof (*ret));
>>>>
>>>> Now here's the tricky part, when i fix both problem, it actually when
>>>> you call this: (memdisk size is 19,180, just in case it matters).
>>>>
>>>> testspeed /memdisk
>>>>
>>>> So there must be a memory corruption somewhere.
>>> You can check for memory corruptions by calling grub_mm_check often
>>> enough in the code.
>> Hi,
>>
>> Thanks for the tip. But when I add a few grub_mm_check and printf here
>> and there, it doesn't halt any more. So this could be a memory
>> overflown issue or even compiler bug, very strange indeed.
> Hi,
>
> Well, actually it does halt with a larger file, so at least the
> behavior is predictable.
Could you try to allocate the buffer for receive/send EFI calls only
once per card? It will result in needless copying but would solve few
DMA issues if network driver is badly coded.


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: Mysterious memory corruption bug

[Bean]

On Wed, May 2, 2012 at 3:56 AM, Vladimir 'φ-coder/phcoder' Serbinenko
<phcoder@gmail.com> wrote:
> On 01.05.2012 21:52, Bean wrote:
>> On Wed, May 2, 2012 at 3:46 AM, Bean <bean123ch@gmail.com> wrote:
>>> On Wed, May 2, 2012 at 3:08 AM, Vladimir 'φ-coder/phcoder' Serbinenko
>>> <phcoder@gmail.com> wrote:
>>>> On 01.05.2012 20:53, Bean wrote:
>>>>> Hi,
>>>>>
>>>>> Thanks to Vladimir's memory patch, it's actually quite easy to
>>>>> reproduce mysterious issue.
>>>>>
>>>>> First, there are two memory leaks in ip.c.
>>>>>
>>>>> It allocates the rsm but never frees it. free_rsm frees its content,
>>>>> but not the pointer itself. You can see it in printmem at ip.c:473
>>>>>       rsm = grub_malloc (sizeof (*rsm));
>>>>>
>>>>> Another problem is at ip.c:594:
>>>>>   return handle_dgram (ret, card, src_hwaddress,
>>>>>                              hwaddress, proto, &source, &dest,
>>>>>                              ttl);
>>>>> here, ret is netbuff. grub_netbuff_alloc get a buffer for both data
>>>>> and header (data go first), so when it frees the data pointer, the
>>>>> header goes away as well. But here, the header is allocated separately
>>>>> so that it's not free using , you can see it from printmem at ip.c:580
>>>>>       ret = grub_malloc (sizeof (*ret));
>>>>>
>>>>> Now here's the tricky part, when i fix both problem, it actually when
>>>>> you call this: (memdisk size is 19,180, just in case it matters).
>>>>>
>>>>> testspeed /memdisk
>>>>>
>>>>> So there must be a memory corruption somewhere.
>>>> You can check for memory corruptions by calling grub_mm_check often
>>>> enough in the code.
>>> Hi,
>>>
>>> Thanks for the tip. But when I add a few grub_mm_check and printf here
>>> and there, it doesn't halt any more. So this could be a memory
>>> overflown issue or even compiler bug, very strange indeed.
>> Hi,
>>
>> Well, actually it does halt with a larger file, so at least the
>> behavior is predictable.
> Could you try to allocate the buffer for receive/send EFI calls only
> once per card? It will result in needless copying but would solve few
> DMA issues if network driver is badly coded.

Hi,

Yeah, I have a patch that save the buffer for later use when there is
no data, it can solve the unnecessary alloc/free loop.

-- 
Best wishes
Bean

Re: Mysterious memory corruption bug

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 459 bytes --]

On 01.05.2012 22:02, Bean wrote:
> Hi,
>
> Yeah, I have a patch that save the buffer for later use when there is
> no data, it can solve the unnecessary alloc/free loop.
No, what I mean: allocate a buffer once for every card and then do
send/recv with only this buffer and copy to/from it when necessary. This
way if the card DMAs the packet to the same buffer it won't corrupt
anything.


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: Mysterious memory corruption bug

[Bean]

On Wed, May 2, 2012 at 4:06 AM, Vladimir 'φ-coder/phcoder' Serbinenko
<phcoder@gmail.com> wrote:
> On 01.05.2012 22:02, Bean wrote:
>> Hi,
>>
>> Yeah, I have a patch that save the buffer for later use when there is
>> no data, it can solve the unnecessary alloc/free loop.
> No, what I mean: allocate a buffer once for every card and then do
> send/recv with only this buffer and copy to/from it when necessary. This
> way if the card DMAs the packet to the same buffer it won't corrupt
> anything.

Hi,

It's not ok with fragmentation, since there could be multiple ethernet
packet for an ip packet, we need to store the buffer for assembling.

-- 
Best wishes
Bean

Re: Mysterious memory corruption bug

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 893 bytes --]

On 01.05.2012 22:09, Bean wrote:
> On Wed, May 2, 2012 at 4:06 AM, Vladimir 'φ-coder/phcoder' Serbinenko
> <phcoder@gmail.com> wrote:
>> On 01.05.2012 22:02, Bean wrote:
>>> Hi,
>>>
>>> Yeah, I have a patch that save the buffer for later use when there is
>>> no data, it can solve the unnecessary alloc/free loop.
>> No, what I mean: allocate a buffer once for every card and then do
>> send/recv with only this buffer and copy to/from it when necessary. This
>> way if the card DMAs the packet to the same buffer it won't corrupt
>> anything.
> Hi,
>
> It's not ok with fragmentation, since there could be multiple ethernet
> packet for an ip packet, we need to store the buffer for assembling.
We use the buffer I said only for actual calls. It's copied to
newly-allocated packet as soon as the call returns.


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: Mysterious memory corruption bug

[Bean]

On Wed, May 2, 2012 at 4:16 AM, Vladimir 'φ-coder/phcoder' Serbinenko
<phcoder@gmail.com> wrote:
> On 01.05.2012 22:09, Bean wrote:
>> On Wed, May 2, 2012 at 4:06 AM, Vladimir 'φ-coder/phcoder' Serbinenko
>> <phcoder@gmail.com> wrote:
>>> On 01.05.2012 22:02, Bean wrote:
>>>> Hi,
>>>>
>>>> Yeah, I have a patch that save the buffer for later use when there is
>>>> no data, it can solve the unnecessary alloc/free loop.
>>> No, what I mean: allocate a buffer once for every card and then do
>>> send/recv with only this buffer and copy to/from it when necessary. This
>>> way if the card DMAs the packet to the same buffer it won't corrupt
>>> anything.
>> Hi,
>>
>> It's not ok with fragmentation, since there could be multiple ethernet
>> packet for an ip packet, we need to store the buffer for assembling.
> We use the buffer I said only for actual calls. It's copied to
> newly-allocated packet as soon as the call returns.

Hi,

Yeah, after more thought, it's doable. We can use a ring buffer for
current active ip frames, and copy ethernet frame to it. This way we
can get rid of the rsm dynamic queue. It can also get rid of tons of
grub_netbuff_free scattered around in various layers.

-- 
Best wishes
Bean

unsubscribe

[Daniel Senderowicz]

[-- Attachment #1: Type: text/plain, Size: 1359 bytes --]

Please unsubscribe

On Wed, 2012-05-02 at 04:34 +0800, Bean wrote:

> On Wed, May 2, 2012 at 4:16 AM, Vladimir 'φ-coder/phcoder' Serbinenko
> <phcoder@gmail.com> wrote:
> > On 01.05.2012 22:09, Bean wrote:
> >> On Wed, May 2, 2012 at 4:06 AM, Vladimir 'φ-coder/phcoder' Serbinenko
> >> <phcoder@gmail.com> wrote:
> >>> On 01.05.2012 22:02, Bean wrote:
> >>>> Hi,
> >>>>
> >>>> Yeah, I have a patch that save the buffer for later use when there is
> >>>> no data, it can solve the unnecessary alloc/free loop.
> >>> No, what I mean: allocate a buffer once for every card and then do
> >>> send/recv with only this buffer and copy to/from it when necessary. This
> >>> way if the card DMAs the packet to the same buffer it won't corrupt
> >>> anything.
> >> Hi,
> >>
> >> It's not ok with fragmentation, since there could be multiple ethernet
> >> packet for an ip packet, we need to store the buffer for assembling.
> > We use the buffer I said only for actual calls. It's copied to
> > newly-allocated packet as soon as the call returns.
> 
> Hi,
> 
> Yeah, after more thought, it's doable. We can use a ring buffer for
> current active ip frames, and copy ethernet frame to it. This way we
> can get rid of the rsm dynamic queue. It can also get rid of tons of
> grub_netbuff_free scattered around in various layers.
> 



[-- Attachment #2: Type: text/html, Size: 1839 bytes --]

Re: unsubscribe

[Gregg Levine]

On Tue, May 1, 2012 at 4:35 PM, Daniel Senderowicz
<daniel@synchrodesign.com> wrote:
> Please unsubscribe
>
> On Wed, 2012-05-02 at 04:34 +0800, Bean wrote:
>
> On Wed, May 2, 2012 at 4:16 AM, Vladimir 'φ-coder/phcoder' Serbinenko
> <phcoder@gmail.com> wrote:
>> On 01.05.2012 22:09, Bean wrote:
>>> On Wed, May 2, 2012 at 4:06 AM, Vladimir 'φ-coder/phcoder' Serbinenko
>>> <phcoder@gmail.com> wrote:
>>>> On 01.05.2012 22:02, Bean wrote:
>>>>> Hi,
>>>>>
>>>>> Yeah, I have a patch that save the buffer for later use when there is
>>>>> no data, it can solve the unnecessary alloc/free loop.
>>>> No, what I mean: allocate a buffer once for every card and then do
>>>> send/recv with only this buffer and copy to/from it when necessary. This
>>>> way if the card DMAs the packet to the same buffer it won't corrupt
>>>> anything.
>>> Hi,
>>>
>>> It's not ok with fragmentation, since there could be multiple ethernet
>>> packet for an ip packet, we need to store the buffer for assembling.
>> We use the buffer I said only for actual calls. It's copied to
>> newly-allocated packet as soon as the call returns.
>
> Hi,
>
> Yeah, after more thought, it's doable. We can use a ring buffer for
> current active ip frames, and copy ethernet frame to it. This way we
> can get rid of the rsm dynamic queue. It can also get rid of tons of
> grub_netbuff_free scattered around in various layers.
>
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

Hello!
Daniel should you really want to do this, please do not send such
messages to the list. Please make use of the header. There you will
find methods to leave this wonderful list.

-----
Gregg C Levine gregg.drwho8@gmail.com
"This signature fought the Time Wars, time and again."