[pacman-dev] Dealing with asprintf failures
The remaining warnings generated with -D_FORTIFY_SOURCE=2 are usages of asprintf in src/pacman/util.c. e.g. util.c: In function ‘display_targets’: util.c:533:12: warning: ignoring return value of ‘asprintf’, declared with attribute warn_unused_result What is the best way to deal with these? One idea is: - asprintf(&str, "%s-%s [%.2f MB]", alpm_pkg_get_name(pkg), - alpm_pkg_get_version(pkg), mbsize); + if(asprintf(&str, "%s-%s [%.2f MB]", alpm_pkg_get_name(pkg), + alpm_pkg_get_version(pkg), mbsize) == -1) { + pm_fprintf(stderr, PM_LOG_ERROR, _("failed to allocate string\n")); + } This prints an error, but does not actually bail, on failure of asprintf. This is an improvement over the current code as we would know where the error occurred, but feels wrong not to exit. Note that several of the functions affected return void, so erroring out is not a simple "return -1". So, any ideas on the best way to approach this? Allan
This prints an error, but does not actually bail, on failure of asprintf. This is an improvement over the current code as we would know where the error occurred, but feels wrong not to exit. Note that several of the functions affected return void, so erroring out is not a simple "return -1".
I don't know any C so sorry If that sounds stupid. Wouldn't creating an errfunc works here? #sudo code: int errfunc(void) { return -1 ; } void somefunc(void) { errfunc() ; } int main(void) { somefunc() ; }
So, any ideas on the best way to approach this?
Allan
On Sun, Jun 27, 2010 at 9:22 AM, Allan McRae <allan@archlinux.org> wrote:
So, any ideas on the best way to approach this?
Counting 13 instances of asprintf, so voting "no" to both suggestions so far. Andres P
On 28/06/10 06:31, Andres P wrote:
On Sun, Jun 27, 2010 at 9:22 AM, Allan McRae<allan@archlinux.org> wrote:
So, any ideas on the best way to approach this?
Counting 13 instances of asprintf, so voting "no" to both suggestions so far.
What does the number of asprintf's have to do with this? Anyway, voting no does not count if you do not provide a better solution. The return value of asprintf needs to be checked when compiling with -D_FORTIFY_SOURCE=2 which I want to use with --enable-debug. So something has to be done... Allan
On Sun, Jun 27, 2010 at 6:45 PM, Allan McRae <allan@archlinux.org> wrote:
On 28/06/10 06:31, Andres P wrote:
On Sun, Jun 27, 2010 at 9:22 AM, Allan McRae<allan@archlinux.org> wrote:
So, any ideas on the best way to approach this?
Counting 13 instances of asprintf, so voting "no" to both suggestions so far.
What does the number of asprintf's have to do with this?
I hope you don't suggest changing each to if(!asprintf(...))
Anyway, voting no does not count if you do not provide a better solution. The return value of asprintf needs to be checked when compiling with -D_FORTIFY_SOURCE=2 which I want to use with --enable-debug. So something has to be done...
You already have a pm_vasprintf wrapper, so I don't see why you couldn't make a asprintf wrapper here. Both of these solutions, the original proposition and writing a wrapper, are ultimately workarounds since the parent functions need to be changed from void so that nobody is reduced to a little shy message log instead of an exit. Why would you change this just to compile with fortify_source when it's showing inherent problems that are more important? btw, http://www.ijs.si/software/snprintf/ Andres P
Instead of throwing away the return value, make a single function handle errors. Signed-off-by: Andres P <aepd87@gmail.com> --- I strongly disagree with making this void, but until all these void funcions get a rework then I guess this is better than outright ignoring. src/pacman/util.c | 33 ++++++++++++++++++++++----------- 1 files changed, 22 insertions(+), 11 deletions(-) diff --git a/src/pacman/util.c b/src/pacman/util.c index de1b162..1b2f451 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -44,6 +44,17 @@ #include "conf.h" #include "callback.h" +/* error handling in a single fn please */ +void vw_asprintf(char **str, const char *fmt, ...) +{ + va_list args; + va_start(args, fmt); + if(vasprintf(str, fmt, args) != 0) { + pm_fprintf(stderr, PM_LOG_ERROR, _("failed to allocate string\n")); + /* goto halt */ + } + va_end(args); +} int trans_init(pmtransflag_t flags) { @@ -530,10 +541,10 @@ void display_targets(const alpm_list_t *pkgs, int install) double mbsize = 0.0; mbsize = alpm_pkg_get_size(pkg) / (1024.0 * 1024.0); - asprintf(&str, "%s-%s [%.2f MB]", alpm_pkg_get_name(pkg), + vw_asprintf(&str, "%s-%s [%.2f MB]", alpm_pkg_get_name(pkg), alpm_pkg_get_version(pkg), mbsize); } else { - asprintf(&str, "%s-%s", alpm_pkg_get_name(pkg), + vw_asprintf(&str, "%s-%s", alpm_pkg_get_name(pkg), alpm_pkg_get_version(pkg)); } targets = alpm_list_add(targets, str); @@ -544,7 +555,7 @@ void display_targets(const alpm_list_t *pkgs, int install) mbisize = isize / (1024.0 * 1024.0); if(install) { - asprintf(&str, _("Targets (%d):"), alpm_list_count(targets)); + vw_asprintf(&str, _("Targets (%d):"), alpm_list_count(targets)); list_display(str, targets); free(str); printf("\n"); @@ -554,7 +565,7 @@ void display_targets(const alpm_list_t *pkgs, int install) printf(_("Total Installed Size: %.2f MB\n"), mbisize); } } else { - asprintf(&str, _("Remove (%d):"), alpm_list_count(targets)); + vw_asprintf(&str, _("Remove (%d):"), alpm_list_count(targets)); list_display(str, targets); free(str); printf("\n"); @@ -588,14 +599,14 @@ static char *pkg_get_location(pmpkg_t *pkg) dburl = alpm_db_get_url(db); if(dburl) { char *pkgurl = NULL; - asprintf(&pkgurl, "%s/%s", dburl, alpm_pkg_get_filename(pkg)); + vw_asprintf(&pkgurl, "%s/%s", dburl, alpm_pkg_get_filename(pkg)); return(pkgurl); } case PM_OP_UPGRADE: return(strdup(alpm_pkg_get_filename(pkg))); default: string = NULL; - asprintf(&string, "%s-%s", alpm_pkg_get_name(pkg), alpm_pkg_get_version(pkg)); + vw_asprintf(&string, "%s-%s", alpm_pkg_get_name(pkg), alpm_pkg_get_version(pkg)); return(string); } } @@ -646,7 +657,7 @@ void print_packages(const alpm_list_t *packages) char *size; double mbsize = 0.0; mbsize = pkg_get_size(pkg) / (1024.0 * 1024.0); - asprintf(&size, "%.2f", mbsize); + vw_asprintf(&size, "%.2f", mbsize); string = strreplace(temp, "%s", size); free(size); free(temp); @@ -793,16 +804,16 @@ int pm_vasprintf(char **string, pmloglevel_t level, const char *format, va_list /* print a prefix to the message */ switch(level) { case PM_LOG_DEBUG: - asprintf(string, "debug: %s", msg); + vw_asprintf(string, "debug: %s", msg); break; case PM_LOG_ERROR: - asprintf(string, _("error: %s"), msg); + vw_asprintf(string, _("error: %s"), msg); break; case PM_LOG_WARNING: - asprintf(string, _("warning: %s"), msg); + vw_asprintf(string, _("warning: %s"), msg); break; case PM_LOG_FUNCTION: - asprintf(string, _("function: %s"), msg); + vw_asprintf(string, _("function: %s"), msg); break; default: break; -- 1.7.1
On Sun, Jun 27, 2010 at 9:26 PM, Andres P <aepd87@gmail.com> wrote:
src/pacman/util.c | 33 ++++++++++++++++++++++----------- 1 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/src/pacman/util.c b/src/pacman/util.c index de1b162..1b2f451 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -44,6 +44,17 @@ #include "conf.h" #include "callback.h"
+/* error handling in a single fn please */ +void vw_asprintf(char **str, const char *fmt, ...) +{ + va_list args; + va_start(args, fmt); + if(vasprintf(str, fmt, args) != 0) {
err, == -1 Andres P
On 28/06/10 11:56, Andres P wrote:
Instead of throwing away the return value, make a single function handle errors.
Signed-off-by: Andres P<aepd87@gmail.com> ---
I strongly disagree with making this void, but until all these void funcions get a rework then I guess this is better than outright ignoring.
Then make it return an int. The functions calling this can be adjusted later, but at least adding the error message is a step in the right direction.
src/pacman/util.c | 33 ++++++++++++++++++++++----------- 1 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/src/pacman/util.c b/src/pacman/util.c index de1b162..1b2f451 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -44,6 +44,17 @@ #include "conf.h" #include "callback.h"
+/* error handling in a single fn please */
Seriously, this is one of the my useless pieces of function documentation I have ever seen.
+void vw_asprintf(char **str, const char *fmt, ...)
Why "vw_"? We have several other similar wrappers called pm_<foo> in util.c already, so stick with that naming for consistency. Also, add the function proto to util.h.
+{ + va_list args; + va_start(args, fmt); + if(vasprintf(str, fmt, args) != 0) {
... oh, I see you flag this issue in the next email
+ pm_fprintf(stderr, PM_LOG_ERROR, _("failed to allocate string\n")); + /* goto halt */ + } + va_end(args); +}
int trans_init(pmtransflag_t flags) { @@ -530,10 +541,10 @@ void display_targets(const alpm_list_t *pkgs, int install) double mbsize = 0.0; mbsize = alpm_pkg_get_size(pkg) / (1024.0 * 1024.0);
- asprintf(&str, "%s-%s [%.2f MB]", alpm_pkg_get_name(pkg), + vw_asprintf(&str, "%s-%s [%.2f MB]", alpm_pkg_get_name(pkg), alpm_pkg_get_version(pkg), mbsize); <snip>
As an aside, it was obvious that this was something I was working on. So duplicating the work is a waste of both of our time. On that note, do not bother resubmitting with my comments addressed as it would make the patch almost identical to what I have locally. Allan
On Mon, Jun 28, 2010 at 12:11 AM, Allan McRae <allan@archlinux.org> wrote:
+/* error handling in a single fn please */
Seriously, this is one of the my useless pieces of function documentation I have ever seen.
Call it wishful thinking?
+void vw_asprintf(char **str, const char *fmt, ...)
Why "vw_"? We have several other similar wrappers called pm_<foo> in util.c already, so stick with that naming for consistency.
Also, add the function proto to util.h.
I hope you don't do that since noone that includes util.h uses asprintf. This wrapper is useless outside util.c for the time being. Obviously prefixing it with pm_ is retarded since, as I said, it gets no use outside were it's defined...
As an aside, it was obvious that this was something I was working on. So duplicating the work is a waste of both of our time. On that note, do not bother resubmitting with my comments addressed as it would make the patch almost identical to what I have locally.
Um, I explicitely said in the patch that I disagreed with it. If that's not indicative that I don't intend for this to be a commit, I don't know what is. I was showing you how to do it with a function without copy pasting everything. Also, good thing that this whole asprintf "issue" made me realize that pacman isn't ansi at all. Andres P
On 28/06/10 14:56, Andres P wrote:
On Mon, Jun 28, 2010 at 12:11 AM, Allan McRae<allan@archlinux.org> wrote:
+/* error handling in a single fn please */
Seriously, this is one of the my useless pieces of function documentation I have ever seen.
Call it wishful thinking?
+void vw_asprintf(char **str, const char *fmt, ...)
Why "vw_"? We have several other similar wrappers called pm_<foo> in util.c already, so stick with that naming for consistency.
Also, add the function proto to util.h.
I hope you don't do that since noone that includes util.h uses asprintf. This wrapper is useless outside util.c for the time being.
Obviously prefixing it with pm_ is retarded since, as I said, it gets no use outside were it's defined...
So, if someone ever wants to use it outside util.c, we need to rename the function and add its header. Call it forward thinking to do it properly the first time. Any decent compiler removes unneeded functions on any include anyway so there is no loss in doing this but there is potential future gain. I'd say not doing that way is "retarded", but really there is no call to start being offensive around here.
As an aside, it was obvious that this was something I was working on. So duplicating the work is a waste of both of our time. On that note, do not bother resubmitting with my comments addressed as it would make the patch almost identical to what I have locally.
Um, I explicitely said in the patch that I disagreed with it. If that's not indicative that I don't intend for this to be a commit, I don't know what is. I was showing you how to do it with a function without copy pasting everything.
No, you said: I strongly disagree with making this void, but until all these void funcions get a rework then I guess this is better than outright ignoring. That tells me that this is a commit up for consideration as it improves the current situation. Also, submitting a whole patch in proper git format indicates that you want it considered. Or were you assuming that I did not understand you previous "make a asprintf wrapper" comment... Allan
On Mon, Jun 28, 2010 at 12:52 AM, Allan McRae <allan@archlinux.org> wrote:
So, if someone ever wants to use it outside util.c, we need to rename the function and add its header. Call it forward thinking to do it properly the first time. Any decent compiler removes unneeded functions on any include anyway so there is no loss in doing this but there is potential future gain.
I'd say not doing that way is "retarded", but really there is no call to start being offensive around here.
Right...
No, you said: I strongly disagree with making this void, but until all these void funcions get a rework then I guess this is better than outright ignoring.
That tells me that this is a commit up for consideration as it improves the current situation. Also, submitting a whole patch in proper git format indicates that you want it considered.
Or were you assuming that I did not understand you previous "make a asprintf wrapper" comment...
Look, hacking on this is fun, but these little husband-wife bickerings are not. I'm gonna have to call it quits. For the record, I meant no offense with my technical remarks. ;) Andres P
participants (3)
-
Allan McRae
-
Andres P
-
Nezmer