Re: [Ipmitool-devel] Code Review - ID: 3600926 - 'lib/ipmi_lanp.c' - check LAN param
On Mon, Mar 11, 2013 at 6:55 PM, Ales Ledvinka aledv...@redhat.com wrote: Oh and noticed the tracker comment about the bigger issue. Yes, get_lan_param_select() may return NULL, yet it's not being checked what the returned value is, resp. whether it's NULL or isn't. NULL reference in 3 ... 2 ... 1 If you are testing with current cvs code I guarantee you will run into a plenty of double-free bugs after the leaks fixes. So if it was related to that... Interesting, but I advise you to put piece of broken code or, even better, SF.net ticket where your mouth is. Because it might seem like it's just a dust coming out of your mouth. Best regards, Zdenek - Original Message - From: Ales Ledvinka aledv...@redhat.com To: Zdenek Styblik zdenek.styb...@gmail.com Cc: ipmitool-devel ipmitool-devel@lists.sourceforge.net Sent: Monday, March 11, 2013 6:32:09 PM Subject: Re: [Ipmitool-devel] Code Review - ID: 3600926 - 'lib/ipmi_lanp.c' - check LAN param Hello, I would ACK it, partially due to backwards compatibility. Rework proposal below. Should not it be checking only 0, 192) and leave the rest to the OEM code? Although I expect this place to be changing soon. The rest is more of a know this part a bit more then really a review. I see the parameters are identified by ipmi_lanp.h enum. This place is being modified also by one oem patch pending on my list. https://bugzilla.redhat.com/attachment.cgi?id=705662 hunk @@ -79,10 +79,17 @@ These are wire protocol id numbers, right? Spec table 23-2 parameter selector. And the 192 and above OEM reserved, Table 23-4. My question really is about the OEM parameters 192 and above with conflicting OEM overlap. Since the code does not seem to be prepared to handle that unless I am overlooking something. If so then the 192+ commands are split according to spec: The OEM is identified according to the Manufacturer ID field returned by the Get Device ID command. - Original Message - From: Zdenek Styblik zdenek.styb...@gmail.com To: ipmitool-devel ipmitool-devel@lists.sourceforge.net Cc: Jim Mank jm...@hp.com, Ales Ledvinka aledv...@redhat.com Sent: Friday, March 8, 2013 9:45:05 PM Subject: Code Review - ID: 3600926 - 'lib/ipmi_lanp.c' - check LAN param Hello all, attached is a diff of proposed change to 'lib/ipmi_lanp.c'. This bug has been reported by Ales Ledvinka a the problem is this ``if (p == NULL)'' condition is never met, because struct is on heap, if I understood and writing it correctly. Comments? Ideas? I've tested it on small snippet code and it seems to work just fine. It's possible such change will be required at more places in 'lib/ipmi_lanp.c'. Thanks, Z. -- Symantec Endpoint Protection 12 positioned as A LEADER in The Forrester Wave(TM): Endpoint Security, Q1 2013 and remains a good choice in the endpoint security space. For insight on selecting the right partner to tackle endpoint security challenges, access the full report. http://p.sf.net/sfu/symantec-dev2dev ___ Ipmitool-devel mailing list Ipmitool-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ipmitool-devel -- Symantec Endpoint Protection 12 positioned as A LEADER in The Forrester Wave(TM): Endpoint Security, Q1 2013 and remains a good choice in the endpoint security space. For insight on selecting the right partner to tackle endpoint security challenges, access the full report. http://p.sf.net/sfu/symantec-dev2dev ___ Ipmitool-devel mailing list Ipmitool-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
Re: [Ipmitool-devel] Code Review - ID: 3600926 - 'lib/ipmi_lanp.c' - check LAN param
Planned though the report/patch I have for the double free does not match current cvs. Contains open tracker items (not yet commited) and does not include some new changes. Unfortunatly my setup to rescan the code needs few fixes so that was just a notice that if you are seeing weird things there might be a well known source already. - Original Message - From: Zdenek Styblik zdenek.styb...@gmail.com To: Ales Ledvinka aledv...@redhat.com Cc: ipmitool-devel ipmitool-devel@lists.sourceforge.net Sent: Tuesday, March 12, 2013 9:27:07 AM Subject: Re: [Ipmitool-devel] Code Review - ID: 3600926 - 'lib/ipmi_lanp.c' - check LAN param On Mon, Mar 11, 2013 at 6:55 PM, Ales Ledvinka aledv...@redhat.com wrote: Oh and noticed the tracker comment about the bigger issue. Yes, get_lan_param_select() may return NULL, yet it's not being checked what the returned value is, resp. whether it's NULL or isn't. NULL reference in 3 ... 2 ... 1 If you are testing with current cvs code I guarantee you will run into a plenty of double-free bugs after the leaks fixes. So if it was related to that... Interesting, but I advise you to put piece of broken code or, even better, SF.net ticket where your mouth is. Because it might seem like it's just a dust coming out of your mouth. Best regards, Zdenek - Original Message - From: Ales Ledvinka aledv...@redhat.com To: Zdenek Styblik zdenek.styb...@gmail.com Cc: ipmitool-devel ipmitool-devel@lists.sourceforge.net Sent: Monday, March 11, 2013 6:32:09 PM Subject: Re: [Ipmitool-devel] Code Review - ID: 3600926 - 'lib/ipmi_lanp.c' - check LAN param Hello, I would ACK it, partially due to backwards compatibility. Rework proposal below. Should not it be checking only 0, 192) and leave the rest to the OEM code? Although I expect this place to be changing soon. The rest is more of a know this part a bit more then really a review. I see the parameters are identified by ipmi_lanp.h enum. This place is being modified also by one oem patch pending on my list. https://bugzilla.redhat.com/attachment.cgi?id=705662 hunk @@ -79,10 +79,17 @@ These are wire protocol id numbers, right? Spec table 23-2 parameter selector. And the 192 and above OEM reserved, Table 23-4. My question really is about the OEM parameters 192 and above with conflicting OEM overlap. Since the code does not seem to be prepared to handle that unless I am overlooking something. If so then the 192+ commands are split according to spec: The OEM is identified according to the Manufacturer ID field returned by the Get Device ID command. - Original Message - From: Zdenek Styblik zdenek.styb...@gmail.com To: ipmitool-devel ipmitool-devel@lists.sourceforge.net Cc: Jim Mank jm...@hp.com, Ales Ledvinka aledv...@redhat.com Sent: Friday, March 8, 2013 9:45:05 PM Subject: Code Review - ID: 3600926 - 'lib/ipmi_lanp.c' - check LAN param Hello all, attached is a diff of proposed change to 'lib/ipmi_lanp.c'. This bug has been reported by Ales Ledvinka a the problem is this ``if (p == NULL)'' condition is never met, because struct is on heap, if I understood and writing it correctly. Comments? Ideas? I've tested it on small snippet code and it seems to work just fine. It's possible such change will be required at more places in 'lib/ipmi_lanp.c'. Thanks, Z. -- Symantec Endpoint Protection 12 positioned as A LEADER in The Forrester Wave(TM): Endpoint Security, Q1 2013 and remains a good choice in the endpoint security space. For insight on selecting the right partner to tackle endpoint security challenges, access the full report. http://p.sf.net/sfu/symantec-dev2dev ___ Ipmitool-devel mailing list Ipmitool-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ipmitool-devel -- Symantec Endpoint Protection 12 positioned as A LEADER in The Forrester Wave(TM): Endpoint Security, Q1 2013 and remains a good choice in the endpoint security space. For insight on selecting the right partner to tackle endpoint security challenges, access the full report. http://p.sf.net/sfu/symantec-dev2dev ___ Ipmitool-devel mailing list Ipmitool-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
Re: [Ipmitool-devel] OEM LAN params was:[Re: Code Review - ID: 3600926 - 'lib/ipmi_lanp.c' - check LAN param]
On Tue, Mar 12, 2013 at 11:12 AM, Ales Ledvinka aledv...@redhat.com wrote: Correct me if I am overlooking something, but the current 192+ codes are unreachable, right? It's seem so, yes. It's annotated as the 1.1 initial revision but I do not see it used nowdays anywhere in the code (macro name/number code). The 192 in hpfwm does not seems to be related. *shrug* So what would be acceptable strategy for the cxoem patch merging? Is it simply replacing the conflicting parameters and extend the lanp as it is now or adding the oem infrastructure? To be honest, I'd like to see the latter. If it's not today, it's going to be tomorrow. Just because some values aren't used and can be replaced doesn't truly solve the problem, does it? My $0.02 USD. Best regards, Zdenek - Original Message - From: Zdenek Styblik zdenek.styb...@gmail.com To: Ales Ledvinka aledv...@redhat.com Cc: Jim Mank jm...@hp.com, ipmitool-devel ipmitool-devel@lists.sourceforge.net Sent: Tuesday, March 12, 2013 9:25:05 AM Subject: Re: Code Review - ID: 3600926 - 'lib/ipmi_lanp.c' - check LAN param On Mon, Mar 11, 2013 at 6:32 PM, Ales Ledvinka aledv...@redhat.com wrote: Hello, I would ACK it, partially due to backwards compatibility. Rework proposal below. I'm sorry, but what? Have you actually thought about the issue you've reported and what the patch does? Should not it be checking only 0, 192) and leave the rest to the OEM code? I don't think so. And what OEM code? I don't see any OEM code there. Although I expect this place to be changing soon. Do you now? Can you tell me when, how and who is going to re-write it? The rest is more of a know this part a bit more then really a review. ? I see the parameters are identified by ipmi_lanp.h enum. This place is being modified also by one oem patch pending on my list. https://bugzilla.redhat.com/attachment.cgi?id=705662 hunk @@ -79,10 +79,17 @@ To be precise about what the patch does is, it adds more parameters. Sadly though, these are in conflict with the current ones. I'd say, tough cookie. These are wire protocol id numbers, right? Spec table 23-2 parameter selector. And the 192 and above OEM reserved, Table 23-4. Yes, that's the one, the table. My question really is about the OEM parameters 192 and above with conflicting OEM overlap. Since the code does not seem to be prepared to handle that unless I am overlooking something. Yes, code doesn't seem to be ready. But you're free to write a patch which makes it ready. And they're only in conflict with each other, but they're just fine as a freedom of implementation by OEM. If so then the 192+ commands are split according to spec: The OEM is identified according to the Manufacturer ID field returned by the Get Device ID command. Yep, that's correct. You should get the OEM ID and the code should do the magic based on this ID. But the magic isn't there. Best regards, Zdenek - Original Message - From: Zdenek Styblik zdenek.styb...@gmail.com To: ipmitool-devel ipmitool-devel@lists.sourceforge.net Cc: Jim Mank jm...@hp.com, Ales Ledvinka aledv...@redhat.com Sent: Friday, March 8, 2013 9:45:05 PM Subject: Code Review - ID: 3600926 - 'lib/ipmi_lanp.c' - check LAN param Hello all, attached is a diff of proposed change to 'lib/ipmi_lanp.c'. This bug has been reported by Ales Ledvinka a the problem is this ``if (p == NULL)'' condition is never met, because struct is on heap, if I understood and writing it correctly. Comments? Ideas? I've tested it on small snippet code and it seems to work just fine. It's possible such change will be required at more places in 'lib/ipmi_lanp.c'. Thanks, Z. -- Symantec Endpoint Protection 12 positioned as A LEADER in The Forrester Wave(TM): Endpoint Security, Q1 2013 and remains a good choice in the endpoint security space. For insight on selecting the right partner to tackle endpoint security challenges, access the full report. http://p.sf.net/sfu/symantec-dev2dev ___ Ipmitool-devel mailing list Ipmitool-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
Re: [Ipmitool-devel] Code Review - ID: 3600926 - 'lib/ipmi_lanp.c' - check LAN param
Z, The patch looks good to me. It certainly fixes the reported problem. -- Jim Mankovich | jm...@hp.com (MST) -- On 3/8/2013 1:45 PM, Zdenek Styblik wrote: Hello all, attached is a diff of proposed change to 'lib/ipmi_lanp.c'. This bug has been reported by Ales Ledvinka a the problem is this ``if (p == NULL)'' condition is never met, because struct is on heap, if I understood and writing it correctly. Comments? Ideas? I've tested it on small snippet code and it seems to work just fine. It's possible such change will be required at more places in 'lib/ipmi_lanp.c'. Thanks, Z. -- Symantec Endpoint Protection 12 positioned as A LEADER in The Forrester Wave(TM): Endpoint Security, Q1 2013 and remains a good choice in the endpoint security space. For insight on selecting the right partner to tackle endpoint security challenges, access the full report. http://p.sf.net/sfu/symantec-dev2dev ___ Ipmitool-devel mailing list Ipmitool-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
