Opened 8 years ago

Closed 8 years ago

#5293 closed Bug (fixed)

Wrong error codes used in upnp.c

Reported by: rb07 Owned by: jordan
Priority: Normal Milestone: 2.77
Component: libtransmission Version: 2.76
Severity: Normal Keywords:
Cc:

Description

It seems that the wrong variable, and wrong error codes are currently being used in tr_upnpDiscover().

It should be something like this:

Index: libtransmission/upnp.c
===================================================================
--- libtransmission/upnp.c      (revision 14017)
+++ libtransmission/upnp.c      (working copy)
@@ -98,11 +98,11 @@
 #elif defined (HAVE_MINIUPNP_15)
     ret = upnpDiscover (msec, NULL, NULL, 0);
 #else
-    ret = UPNPCOMMAND_UNKNOWN_ERROR;
+    ret = UPNPDISCOVER_UNKNOWN_ERROR;
 #endif

-    if (ret != UPNPCOMMAND_SUCCESS)
-        tr_logAddNamedDbg (getKey (), "upnpDiscover failed (errno %d - %s)", err, tr_strerror (err));
+    if (err != UPNPDISCOVER_SUCCESS)
+        tr_logAddNamedDbg (getKey (), "upnpDiscover failed (errno %d - %s)", errno, tr_strerror (errno));

     return ret;
 }

this is assuming errno is preserved by the underlying function, since its not touched, and the value returned in err doesn't have the OS error code.

BTW version 1.8 of miniupnpc was released this week, no changes w.r.t. this problem or the API (except perhaps to change the guards to include the newer versions).

Attachments (1)

010-upnp.diff (794 bytes) - added by rb07 8 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 8 years ago by jordan

  • Status changed from new to assigned

comment:2 Changed 8 years ago by jordan

  • Milestone changed from None Set to 2.80
  • Resolution set to fixed
  • Status changed from assigned to closed

if we're going to do the symbolic values of "err" correctly, we shouldn't initialize it to zero either. :)

r14020: "(libT) #5293 'Wrong error codes used in upnp.c': patch by rb07"

comment:3 Changed 8 years ago by jordan

  • Milestone changed from 2.80 to 2.77

r14021: "(2.7x) backport r14020 to fix bug #5278 in 2.7x"

comment:4 Changed 8 years ago by taem

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:5 Changed 8 years ago by taem

Hi,

Can't build the code with r14020

gcc -DPACKAGE_NAME=\"transmission\" -DPACKAGE_TARNAME=\"transmission\" -DPACKAGE_VERSION=\"2.76+\" -DPACKAGE_STRING=\"transmission\ 2.76+\" -DPACKAGE_BUGREPORT=\"http://trac.transmissionbt.com/newticket\" -DPACKAGE_URL=\"\" -DPACKAGE=\"transmission\" -DVERSION=\"2.76+\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -DSTDC_HEADERS=1 -DTIME_WITH_SYS_TIME=1 -DHAVE_STDBOOL_H=1 -DHAVE_ICONV_OPEN=1 -DHAVE_PREAD=1 -DHAVE_PWRITE=1 -DHAVE_DAEMON=1 -DHAVE_DIRNAME=1 -DHAVE_BASENAME=1 -DHAVE_STRCASECMP=1 -DHAVE_LOCALTIME_R=1 -DHAVE_FALLOCATE64=1 -DHAVE_POSIX_FALLOCATE=1 -DHAVE_MEMMEM=1 -DHAVE_STRSEP=1 -DHAVE_STRTOLD=1 -DHAVE_SYSLOG=1 -DHAVE_VALLOC=1 -DHAVE_GETPAGESIZE=1 -DHAVE_POSIX_MEMALIGN=1 -DHAVE_STATVFS=1 -DHAVE_MKDTEMP=1 -DHAVE_PTHREAD=1 -DHAVE__TMP_DUMMY1_ZLIB_H=1 -DHAVE_ZLIB=1 -D_FILE_OFFSET_BITS=64 -DHAVE_LSEEK64=1 -DHAVE_GETMNTENT=1 -DHAVE_DECL_POSIX_FADVISE=1 -DHAVE_POSIX_FADVISE=1 -DWITH_INOTIFY=1 -DHAVE_SYS_STATVFS_H=1 -DHAVE_XFS_XFS_H=1 -DWITH_UTP=1 -DHAVE_MINIUPNP_15=1 -DSYSTEM_MINIUPNP=1 -DHAVE_LIBINTL_H=1 -DGETTEXT_PACKAGE=\"transmission-gtk\" -DHAVE_LOCALE_H=1 -DHAVE_LC_MESSAGES=1 -DHAVE_BIND_TEXTDOMAIN_CODESET=1 -DHAVE_GETTEXT=1 -DHAVE_DCGETTEXT=1 -DENABLE_NLS=1 -I.  -I.. -D__TRANSMISSION__ -DPACKAGE_DATA_DIR=\""/usr/share"\" -D_FORTIFY_SOURCE=2 -I../third-party/dht -I../third-party/ -pthread  -g -O2 -fPIE -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security -g -O0 -std=gnu99 -ggdb3 -Wall -W -Wpointer-arith -Wformat-security -Wcast-align -Wundef -Wcast-align -Wstrict-prototypes -Wmissing-declarations -Wmissing-format-attribute -Wredundant-decls -Wnested-externs -Wunused-parameter -Wwrite-strings -Winline -Wfloat-equal -Wextra -Wdeclaration-after-statement -Winit-self -Wvariadic-macros -c upnp.c
upnp.c: In function ‘tr_upnpDiscover’:
upnp.c:94:15: error: ‘UPNPDISCOVER_SUCCESS’ undeclared (first use in this function)
upnp.c:94:15: note: each undeclared identifier is reported only once for each function it appears in
make[3]: *** [upnp.o] Error 1
make[3]: Leaving directory `/home/taem/src/transmission/transmission/libtransmission'
make[2]: *** [all-recursive] Error 1
make[2]: Leaving directory `/home/taem/src/transmission/transmission'
dh_auto_build: make -j1 returned exit code 2
make[1]: *** [override_dh_auto_build] Error 2
make[1]: Leaving directory `/home/taem/src/transmission/transmission'
make: *** [build] Error 2
$ dpkg -l | grep miniupnp
ii  libminiupnpc-dev                      1.5-2                              i386         UPnP IGD client lightweight library development files
ii  libminiupnpc5                         1.5-2                              i386         UPnP IGD client lightweight library

comment:6 Changed 8 years ago by rb07

Oops! upnp.c patch take two (added).

But looking at miniupnc's code, in particular their command line tool: upnpc.c, it doesn't use errno at all, just test if ret is NULL, and print their error code (which is one of 3: unknown-, socket-, memory- error). I don't know if logging errno will be usefull.

For debugging purposes the return from UPNP_GetValidIGD() is more interesting, its used to show:

case 1:
	printf("Found valid IGD : %s\n", urls.controlURL);
	break;
case 2:
	printf("Found a (not connected?) IGD : %s\n", urls.controlURL);
	printf("Trying to continue anyway\n");
	break;
case 3:
	printf("UPnP device found. Is it an IGD ? : %s\n", urls.controlURL);
	printf("Trying to continue anyway\n");
	break;
default:
	printf("Found device (igd ?) : %s\n", urls.controlURL);
	printf("Trying to continue anyway\n");

Changed 8 years ago by rb07

comment:7 Changed 8 years ago by taem

rb07, the patch fixed FTBFS. Thanks.

comment:8 Changed 8 years ago by jordan

  • Resolution set to fixed
  • Status changed from reopened to closed

rb07, thanks for the patch. taem, thanks for the testing :)

r14026: #5293 'Wrong error codes used in upnp.c': revised patch by rb07 to accommodate older versions of miniupnpc

r14027: (2.7x) backport r14026 for #5293

Note: See TracTickets for help on using tickets.