In psb_intel_lvds_init(), if we fail to allocate memory for 'psb_intel_connector' we free the memory we previously allocated for 'psb_intel_encoder', but we then proceed to use that free'd pointer when we do 'psb_intel_encoder->dev_priv = lvds_priv;'.
I believe the proper way to handle this is to simply return after the allocation for 'psb_intel_connector' has failed. That is what this patch does.
While I was there anyway, I also removed the pointless 'if (psb_intel_connector)' before freeing it at the 'failed_connector:' label - kfree() deals gracefully with NULL pointers, so it is not needed.
Signed-off-by: Jesper Juhl jj@chaosbits.net --- drivers/gpu/drm/gma500/psb_intel_lvds.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
I considered adding a new label at the end and use a goto instead, but settled on this minimal solution. If consolidating all exits from the function is prefered then I can rework the patch to do that instead. Just let me know.
Patch is compile tested only since I don't have the hardware to actually test it properly.
diff --git a/drivers/gpu/drm/gma500/psb_intel_lvds.c b/drivers/gpu/drm/gma500/psb_intel_lvds.c index a25e4ca..7d23ef9 100644 --- a/drivers/gpu/drm/gma500/psb_intel_lvds.c +++ b/drivers/gpu/drm/gma500/psb_intel_lvds.c @@ -725,6 +725,7 @@ void psb_intel_lvds_init(struct drm_device *dev, if (!psb_intel_connector) { kfree(psb_intel_encoder); dev_err(dev->dev, "psb_intel_connector allocation error\n"); + return; }
lvds_priv = kzalloc(sizeof(struct psb_intel_lvds_priv), GFP_KERNEL); @@ -862,7 +863,6 @@ failed_blc_i2c: drm_encoder_cleanup(encoder); drm_connector_cleanup(connector); failed_connector: - if (psb_intel_connector) - kfree(psb_intel_connector); + kfree(psb_intel_connector); }
On Sat, 14 Jan 2012, Jesper Juhl wrote:
In psb_intel_lvds_init(), if we fail to allocate memory for 'psb_intel_connector' we free the memory we previously allocated for 'psb_intel_encoder', but we then proceed to use that free'd pointer when we do 'psb_intel_encoder->dev_priv = lvds_priv;'.
I believe the proper way to handle this is to simply return after the allocation for 'psb_intel_connector' has failed. That is what this patch does.
Ok, so I just noticed that we may also leak 'psb_intel_encoder' if we 'goto failed_connector;'. Might as well fix that as well in the same patch.
So please just ignore this one. I'll submit a new one in a little while that handles both leaks.
In psb_intel_lvds_init(), if we fail to allocate memory for 'psb_intel_connector' we free the memory we previously allocated for 'psb_intel_encoder', but we then proceed to use that free'd pointer when we do 'psb_intel_encoder->dev_priv = lvds_priv;'.
We may also leak the memory we allocated for 'psb_intel_encoder' if we 'goto failed_connector;' and the variable goes out of scope.
While I was there anyway, I also removed the pointless 'if (psb_intel_connector)' before freeing it at the 'failed_connector:' label - kfree() deals gracefully with NULL pointers, so it is not needed.
Signed-off-by: Jesper Juhl jj@chaosbits.net --- drivers/gpu/drm/gma500/psb_intel_lvds.c | 9 ++++----- 1 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/gma500/psb_intel_lvds.c b/drivers/gpu/drm/gma500/psb_intel_lvds.c index a25e4ca..0a43758 100644 --- a/drivers/gpu/drm/gma500/psb_intel_lvds.c +++ b/drivers/gpu/drm/gma500/psb_intel_lvds.c @@ -713,7 +713,6 @@ void psb_intel_lvds_init(struct drm_device *dev,
psb_intel_encoder = kzalloc(sizeof(struct psb_intel_encoder), GFP_KERNEL); - if (!psb_intel_encoder) { dev_err(dev->dev, "psb_intel_encoder allocation error\n"); return; @@ -721,10 +720,9 @@ void psb_intel_lvds_init(struct drm_device *dev,
psb_intel_connector = kzalloc(sizeof(struct psb_intel_connector), GFP_KERNEL); - if (!psb_intel_connector) { - kfree(psb_intel_encoder); dev_err(dev->dev, "psb_intel_connector allocation error\n"); + goto failed_encoder; }
lvds_priv = kzalloc(sizeof(struct psb_intel_lvds_priv), GFP_KERNEL); @@ -862,7 +860,8 @@ failed_blc_i2c: drm_encoder_cleanup(encoder); drm_connector_cleanup(connector); failed_connector: - if (psb_intel_connector) - kfree(psb_intel_connector); + kfree(psb_intel_connector); +failed_encoder: + kfree(psb_intel_encoder); }
On Sat, Jan 14, 2012 at 10:15 PM, Jesper Juhl wrote:
In psb_intel_lvds_init(), if we fail to allocate memory for 'psb_intel_connector' we free the memory we previously allocated for 'psb_intel_encoder', but we then proceed to use that free'd pointer when we do 'psb_intel_encoder->dev_priv = lvds_priv;'.
We may also leak the memory we allocated for 'psb_intel_encoder' if we 'goto failed_connector;' and the variable goes out of scope.
While I was there anyway, I also removed the pointless 'if (psb_intel_connector)' before freeing it at the 'failed_connector:' label - kfree() deals gracefully with NULL pointers, so it is not needed.
My bad, thanks for fixing
On Tue, 17 Jan 2012, Patrik Jakobsson wrote:
On Sat, Jan 14, 2012 at 10:15 PM, Jesper Juhl wrote:
In psb_intel_lvds_init(), if we fail to allocate memory for 'psb_intel_connector' we free the memory we previously allocated for 'psb_intel_encoder', but we then proceed to use that free'd pointer when we do 'psb_intel_encoder->dev_priv = lvds_priv;'.
We may also leak the memory we allocated for 'psb_intel_encoder' if we 'goto failed_connector;' and the variable goes out of scope.
While I was there anyway, I also removed the pointless 'if (psb_intel_connector)' before freeing it at the 'failed_connector:' label - kfree() deals gracefully with NULL pointers, so it is not needed.
My bad, thanks for fixing
No problem :-) Now I just hope that it will get merged :-)
dri-devel@lists.freedesktop.org
-
Jesper Juhl -
Patrik Jakobsson