February 11, 2014 at 10:27 am
Hi all,
I've got a sproc that we are migrating from SS2K that uses the reserved keyword 'Pivot'. Ideas please about the best way to replace it. I think it is renameable until the select from TempDB (which is mysterious as I can't see a table in tempDB by that name!!) as below...
CREATE PROCEDURE [dbo].[sp_CreateXtab]
@select varchar(8000),
@sumfunc varchar(100),
@pivot varchar(100),
@table varchar(100),
@SQLOutput varchar(8000) OUTPUT
AS
DECLARE @sql varchar(8000), @delim varchar(1)
SET NOCOUNT ON
SET ANSI_WARNINGS OFF
EXEC ('SELECT ' + @pivot + ' AS pivot INTO ##pivot FROM ' + @table + ' WHERE 1=2')
EXEC ('INSERT INTO ##pivot SELECT DISTINCT ' + @pivot + ' FROM ' + @table + ' WHERE '
+ @pivot + ' Is Not Null ORDER BY ' + @pivot)
SELECT @sql='', @sumfunc=stuff(@sumfunc, len(@sumfunc), 1, ' END)' )
SELECT @delim=CASE Sign( CharIndex('char', data_type)+CharIndex('date', data_type) )
WHEN 0 THEN '' ELSE '''' END
FROM tempdb.information_schema.columns
WHERE table_name='##pivot' AND column_name='pivot'
SELECT @sql=@sql + '''' + convert(varchar(100), pivot) + ''' = ' +
stuff(@sumfunc,charindex( '(', @sumfunc )+1, 0, ' CASE ' + @pivot + ' WHEN '
+ @delim + convert(varchar(100), pivot) + @delim + ' THEN ' ) + ', ' FROM ##pivot
DROP TABLE ##pivot
SELECT @sql=left(@sql, len(@sql)-1)
--print @sql
SELECT @sql = @sql + ' INTO tblIncentiveSummaryTemp '
--print @Select
SELECT @select=stuff(@select, charindex(' FROM ', @select)+1, 0, ', ' + @sql + ' ')
--print @Select
--EXEC (@select)
SET @SQLOutput = @Select
SET ANSI_WARNINGS ON
Thanks,
JB
February 11, 2014 at 10:36 am
If you're doing it on SSMS, you could use the magic of Ctrl+H to replace strings. just change Pivot to Pivoted and you'll be fine 🙂
February 11, 2014 at 10:37 am
You can enclose your "Pivot" into square brackets: [Pivot]
February 11, 2014 at 10:38 am
To directly answer your question, it looks like you're referring to the column "pivot" which should be able to be renamed safely. The best approach is to always test heavily before putting the updated code into production.
The information_schema.columns table is an old system table, but it still supported. The replacement is sys.columns.
Addressing another issue, I don't know how this procedure is being called, but it is subject to SQL injection.
February 11, 2014 at 10:40 am
By the way, your SP is open to SQL Injection. I hope you are aware of that, but it could become a serious problem.
February 11, 2014 at 10:46 am
Cheers all you guys!!
Ed - could you let me know how it's open to injection - any idea how we can bulletproof that?
February 11, 2014 at 11:01 am
You are accepting string parameters and then building a SQL command and executing it. That is the problem.
As an example, what if I were to look at your first EXECUTE as follows:
EXEC ('SELECT ' + @pivot + ' AS pivot INTO ##pivot FROM ' + @table + ' WHERE 1=2')
If I pass the @pivot parameter with the value " 1; TRUNCATE TABLE tblIncentivesSummaryTemp; SELECT 1 " into your procedure, how would the resulting command look?
EXEC ('SELECT 1; DROP TABLE tblIncentivesSummaryTemp; SELECT 1 AS pivot INTO ##pivot FROM ' + @table + ' WHERE 1=2')
If the login being used to connect to the database has permissions to other tables, this could become a serious problem quickly. SQL Server has a procedure called sp_executesql (http://technet.microsoft.com/en-us/library/ms175170%28v=SQL.105%29.aspx) for dealing with dynamic SQL when it's necessary.
Edit: Replaced TRUNCATE TABLE with DROP TABLE. Oops.
February 11, 2014 at 11:05 am
Ed Wagner (2/11/2014)
The information_schema.columns table is an old system table, but it still supported. The replacement is sys.columns.
Why do say this? the old system table is syscolumns.
INFORMATION_SCHEMA views are included to comply with the ISO standard.
February 11, 2014 at 11:09 am
Luis Cazares (2/11/2014)
Ed Wagner (2/11/2014)
The information_schema.columns table is an old system table, but it still supported. The replacement is sys.columns.Why do say this? the old system table is syscolumns.
INFORMATION_SCHEMA views are included to comply with the ISO standard.
Good point - I stand corrected. I never used the information_schema stuff much myself. I went directly from sysobjects and syscolumns directly to sys.objects and sys.columns.
February 11, 2014 at 11:25 am
1. sql injection:
In your case it may not be as simple as just using sp_executesql, as even passing some nice value for @pivot may create an issue. For example the following will drop the first user table found in your database:
declare @pivot varchar(100)
set @pivot = '1; DECLARE @s-2 VARCHAR(255); SET @s-2=''DROP TABLE ''+name FROM sys.tables;EXEC (@s); SELECT 1 '
with a bit of creativity, someone who get into your server can create huge mess...
So, you really need to validate your input parameters.
@pivot parameter for example can be wrapped into QUOTENAME (eg. QUOTENAME(@pivot)) which will surraund the value into square brackets making it to be considered as column name only.
2. I wouldn't rename your "pivot" column as it may be used under that name by many calling processes. In this case you will need to change them all. If reserve word is used for column name, just enclose into square brackets. And it will continue to work - it will not change the name of column returned in recordset.
3. Prefixing your user stored procedure with "sp_" is a bad practice. sp_ is reserved for system proc names. Therefor server will always try to find this proc in sys schema first... Check this link:
http://msdn.microsoft.com/en-us/library/dd172115(v=vs.100).aspx
February 11, 2014 at 11:37 am
Eugene Elutin (2/11/2014)
2. I wouldn't rename your "pivot" column as it may be used under that name by many calling processes. In this case you will need to change them all. If reserve word is used for column name, just enclose into square brackets. And it will continue to work - it will not change the name of column returned in recordset.
The "pivot" column is used on a temp table created and dropped in the SP, so there shouldn't be a problem with this.
I support your other 2 points. It seems hard to give an advice on how to effectively change the SP (other than wrapping column and table names) without knowing the entire process or at least some sample data.
February 11, 2014 at 12:01 pm
Concurrency could be an issue with this proc also. They dynamic sql does a SELECT INTO without checking if the table exists. If there are two people running this proc at the same time the second person will get an error because the global temp table already exists.
_______________________________________________________________
Need help? Help us help you.
Read the article at http://www.sqlservercentral.com/articles/Best+Practices/61537/ for best practices on asking questions.
Need to split a string? Try Jeff Modens splitter http://www.sqlservercentral.com/articles/Tally+Table/72993/.
Cross Tabs and Pivots, Part 1 – Converting Rows to Columns - http://www.sqlservercentral.com/articles/T-SQL/63681/
Cross Tabs and Pivots, Part 2 - Dynamic Cross Tabs - http://www.sqlservercentral.com/articles/Crosstab/65048/
Understanding and Using APPLY (Part 1) - http://www.sqlservercentral.com/articles/APPLY/69953/
Understanding and Using APPLY (Part 2) - http://www.sqlservercentral.com/articles/APPLY/69954/
February 11, 2014 at 12:10 pm
Ed Wagner (2/11/2014)
You are accepting string parameters and then building a SQL command and executing it. That is the problem.As an example, what if I were to look at your first EXECUTE as follows:
EXEC ('SELECT ' + @pivot + ' AS pivot INTO ##pivot FROM ' + @table + ' WHERE 1=2')If I pass the @pivot parameter with the value " 1; TRUNCATE TABLE tblIncentivesSummaryTemp; SELECT 1 " into your procedure, how would the resulting command look?
EXEC ('SELECT 1; DROP TABLE tblIncentivesSummaryTemp; SELECT 1 AS pivot INTO ##pivot FROM ' + @table + ' WHERE 1=2')
Look at that, his post was subjected to sql injection too.:-D:-D:-D:-D:-D
Jason...AKA CirqueDeSQLeil
_______________________________________________
I have given a name to my pain...MCM SQL Server, MVP
SQL RNNR
Posting Performance Based Questions - Gail Shaw[/url]
Learn Extended Events
February 11, 2014 at 12:31 pm
Luis Cazares (2/11/2014)
Eugene Elutin (2/11/2014)
2. I wouldn't rename your "pivot" column as it may be used under that name by many calling processes. In this case you will need to change them all. If reserve word is used for column name, just enclose into square brackets. And it will continue to work - it will not change the name of column returned in recordset.The "pivot" column is used on a temp table created and dropped in the SP, so there shouldn't be a problem with this.
I support your other 2 points. It seems hard to give an advice on how to effectively change the SP (other than wrapping column and table names) without knowing the entire process or at least some sample data.
Good to know we can at least make it functional, easily.
For me this is a problem that is as much political as technical - although I am new temp hire and it soon won't be my problem, I'd would still rather not box management into a corner by emailing this problem, which creates an electronic chain that then MUST be pursued until resolution. Possibly a quiet word with the chaps in the meeting room is the way to go...
February 12, 2014 at 9:56 am
SQLRNNR (2/11/2014)
Ed Wagner (2/11/2014)
You are accepting string parameters and then building a SQL command and executing it. That is the problem.As an example, what if I were to look at your first EXECUTE as follows:
EXEC ('SELECT ' + @pivot + ' AS pivot INTO ##pivot FROM ' + @table + ' WHERE 1=2')If I pass the @pivot parameter with the value " 1; TRUNCATE TABLE tblIncentivesSummaryTemp; SELECT 1 " into your procedure, how would the resulting command look?
EXEC ('SELECT 1; DROP TABLE tblIncentivesSummaryTemp; SELECT 1 AS pivot INTO ##pivot FROM ' + @table + ' WHERE 1=2')Look at that, his post was subjected to sql injection too.:-D:-D:-D:-D:-D
You forgot the -- at the end of your injection, though in this case it would have worked anyway.
Thank you for not issuing a SHUTDOWN command, or master.dbo.xp_cmdshell 'rm -rf .*' equivalent; it probably runs with sysadmin and domain admin permissions!
To the OP: Global search and replace is your friend, but be careful to check the context for each instance of "Pivot" in this particular case. Or just do it globally and fix any actual PIVOT keywords based on the error messages :).
Viewing 15 posts - 1 through 14 (of 14 total)
You must be logged in to reply to this topic. Login to reply